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

fix inadvertent re-renders when using Component instead of element #10287

Merged
merged 7 commits into from
Apr 11, 2023

Conversation

brophdawg11
Copy link
Contributor

Closes #10283

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2023

🦋 Changeset detected

Latest commit: 283aa48

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@brophdawg11 brophdawg11 linked an issue Mar 29, 2023 that may be closed by this pull request
@brophdawg11 brophdawg11 self-assigned this Mar 29, 2023
detectErrorBoundary?: DetectErrorBoundaryFunction;
enhanceAgnosticRoute?: EnhanceAgnosticRouteFunction;
Copy link
Member

@markdalgleish markdalgleish Mar 29, 2023

Choose a reason for hiding this comment

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

I feel like this name could be clearer, e.g. resolveRouteProperties, resolveCustomRouteProperties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely open to bike-shedding here. I kinda like indicating that this function is what takes us from framework-agnostic to framework-aware. resolveFrameworkAwareRouteProperties is a mouthful though 😕 . resolveRouteFrameworkProperties or resolveRouteRenderProperties maybe?

Do you think the word "resolve" carries an incorrect connotation that this is how route.lazy stuff gets applied? That's not quite correct since this is a second pass after we apply the route.lazy props.

Longer term, I could even see a future where we decide that Component/ErrorBoundary are framework-agnostic enough (in todays component-based framework landscape, and as opposed to element/errorElement which felt super react-specific) and maybe we just type them as any in the router and that would eliminate hasErrorBoundary entirely. Then we overload the types in the router layer with React.Component etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could also be a future flag, but it's an implementation detail of RR and only newly (6.9.0) public API in @remix-run/router so on the fence as to whether it's worth a formal future flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bikeshedded and currently thinking mapRouteProperties

brophdawg11 and others added 2 commits April 10, 2023 11:47
Co-authored-by: Mark Dalgleish <mark.john.dalgleish@gmail.com>
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.11.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Component causing incorrect re-renders
3 participants