Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update all components in exercises/solutions to use default exports #11

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

kevinb-khan
Copy link
Contributor

Summary:

This will avoid situations where the exercise could be using the original named export even though the developer has memoized the component.

This PR also updates the eslint settings so that devs don't see warnings about certain patterns not working with HMR.

I've also modified the solution in lesson 4 to be even more performant.

Issue: None

Test plan:

  • yarn dev
  • see that the site renders without error
  • check that lesson 4 behaves as expected with the solution being even more performant than before

@kevinb-khan kevinb-khan self-assigned this Jul 3, 2024
@kevinb-khan kevinb-khan requested a review from beaesguerra July 3, 2024 16:23
Copy link
Member

@beaesguerra beaesguerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

fib({n}) = {fib(n)}
</p>
);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was poking around some more! We don't use the arePropsEqual arg here because none of the props are objects or functions, right?

Also, since the Fib component is memoized now, I was thinking the component wouldn't re-render if the props (the n value) didn't change. The visual rendering feature shows that the Fib component gets re-rendered whenever the button is pressed, but the profiler says the Fib component wasn't re-rendered. So the visual rendering feature might not be accurate at times!

Screen.Recording.2024-07-03.at.1.35.35.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'll have a closer look tomorrow. Thanks for the video recording.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing the same behaviour. I'll make a note about it in the preliminaries.

@kevinb-khan kevinb-khan changed the base branch from revise-lesson-1 to main July 4, 2024 21:54
@kevinb-khan kevinb-khan merged commit 321f877 into main Jul 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants