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

[Bug]: Component prop stopped working on v6.11.0 #10416

Closed
diegohaz opened this issue Apr 28, 2023 · 9 comments · Fixed by #10434
Closed

[Bug]: Component prop stopped working on v6.11.0 #10416

diegohaz opened this issue Apr 28, 2023 · 9 comments · Fixed by #10434
Labels

Comments

@diegohaz
Copy link

What version of React Router are you using?

6.11.0

Steps to Reproduce

Visit https://stackblitz.com/edit/github-ceeupc?file=src%2FApp.tsx

Expected Behavior

Route components passed to the Component prop are rendered:
https://stackblitz.com/edit/github-ceeupc-vzvsw6?file=src%2FApp.tsx

Actual Behavior

Route components passed to the Component prop aren't rendered.

@diegohaz diegohaz added the bug label Apr 28, 2023
@timdorr
Copy link
Member

timdorr commented Apr 29, 2023

@brophdawg11 Did #10287 break this?

@JasperHorn
Copy link

I was wondering why my application suddenly wasn't working the way it should anymore...

@brophdawg11
Copy link
Contributor

Hm, yeah, this is a bit of a miscommunication. Component/ErrorBoundary were really only intended for RouterProvider usages but we weren't clear on that in release notes or docs 😕 . The initial implementation did technically allow usages with BrowserRouter, but at the expense of re-rendering every single time even if nothing changes due to how React works under the hood.

For example, Layout will not re-render when navigating between Home/About here (assuming it doesn't pull from RR contexts) because React.createElement is called initially up front and the same ReactElement instance can be re-used across renders.

<Routes>
  <Route path="/" element={<Layout />}>
    <Route index element={<Home />} />
    <Route path="about" element={<About />} />
  </Route>
</Route>

However, if you use Component, then Layout will re-render on every navigation no matter what. This is because React.createElement is called at render time so we create a new ReactElement identity every time and it can never re-use across renders.

<Routes>
  <Route path="/" Component={Layout}>
    <Route index Component={Home} />
    <Route path="about" Component={About} />
  </Route>
</Route>

The reason Component can work in RouterProvider is that we can internally call createElement up front and store the ReactElement instance inside the router so it can avoid re-renders.

We could add support back - but it's a pretty significant de-optimization to use Component in BrowserRouter apps so I don't think we necessarily want to encourage that. But in the spirit of SemVer, this probably would fall under regression and deserve a fix.

@diegohaz and @JasperHorn What do you folks think? Would you want Component support back knowing the performance implications?

@diegohaz
Copy link
Author

diegohaz commented May 2, 2023

For my use case, I can move back to element. I used Component because it seemed like a more straightforward API. Thanks for the clarification.

@JasperHorn
Copy link

I was working on an older application that I hadn't written myself, and I had recently moved it to recent version of react-router. For this, I can move to using element without too much effort - the effort was in finding out why it wasn't working.

You could revert in 6.11.1 and continue in 7.0.0. But I also see why you might not want to bump the major version just for this...

@happylolonly
Copy link

happylolonly commented May 2, 2023

We also had not obvious issue while build, I would also use Component, it looks more clear.

cybercongress/cyb-ts@64cd4dc

In docs it is written as it is the same behavior.
image

@brophdawg11 brophdawg11 self-assigned this May 2, 2023
@brophdawg11 brophdawg11 linked a pull request May 2, 2023 that will close this issue
@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label May 2, 2023
@brophdawg11 brophdawg11 removed their assignment May 2, 2023
@brophdawg11
Copy link
Contributor

This is fixed by #10434 and should be available once 6.11.1 is released

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

🤖 Hello there,

We just published version 6.11.1-pre.0 which involves this issue. 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

github-actions bot commented May 3, 2023

🤖 Hello there,

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

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants