fix(react-router): remove unnecessary load call after redirect
#1662
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e98498c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
|
can you add a test for this? |
|
@schiller-manuel I tried making a test but I'm getting some error at type level, and I also can't replicate the issue within the test itself when the patch is not applied 🤔 hitting a bit of a wall right now, you have any ideas? |
|
Just wanted to confirm that running this PR as a patch does fix my context issues 🥳. Great work! Edit: my issue partially came back, maybe 2 different instances of context undefined in my app |
|
Tested this PR locally and it doesn't seem to address the issue mentioned in #1650. |
IIRC it did fix the issue in the reproduction template for me. However context seems to be very delicate so it might only have fixed this partially. I should have some bandwidth to check back on this in the weekend or early next week (and redo tests based on #1708). |
|
If it helps, I've got a test setup in #1713 that correctly break the route context during redirection. |
|
@SeanCassiere Okay, so I tested this again and it does seem to fix the issue in the reproduction template for me: however, on a more careful look I see this controlled promise always getting rejected after the redirect, and I think the cause is likely this extra load call. In the reproduction case we're navigating twice: once on initialization, and once more on redirect. However, the Now, you've already included the previous changes here in #1713 via 6c1f633: these are good to have but they're probably not the real culprit here, so I'm changing this PR to only address this issue's likely root cause. EDIT: Applying this patch on top of #1713: |
freshgiammi
changed the title
load call after redirect
The patch implemented by @freshgiammi in #1662 Co-Authored-By: Gianmarco Rengucci <16855748+freshgiammi@users.noreply.github.com>
|
@freshgiammi I've applied your fixes to #1713 and it looks good! 🎉 |
|
@SeanCassiere Great! I'll close this PR then and we can merge it via #1713 👍 |
…lled after a redirection (#1713) * test(react-router): adjust the test names to accomodate the on-navigate stuff * test(react-router): breaking route context * test(react-router): test cases for redirects breaking * test(react-router): use a bit more sleep * test(react-router): a bit clearer on whats being tested * test(react-router): check the `window.location.pathname` in the tests * fix(react-router): a single test is now working, dont know what that means * test: make sure StrictMode is ON * test(react-router): this is no longer needed since we are doing this in routeContext.test.tsx * test(react-router): confirm StrictMode on * test(react-router): adjust the test names to accomodate the on-navigate stuff * test(react-router): breaking route context * test(react-router): test cases for redirects breaking * test(react-router): use a bit more sleep * test(react-router): a bit clearer on whats being tested * test(react-router): check the `window.location.pathname` in the tests * fix(react-router): a single test is now working, dont know what that means * test: make sure StrictMode is ON * test(react-router): this is no longer needed since we are doing this in routeContext.test.tsx * test(react-router): confirm StrictMode on * test: wrap fireEvent calls in `act` * chore: readability * fix: i dont know why, but this fixes the tests * test: turn these back on * fix: lets not change too much * fix: bit closer to what it was before * chore: not over updating reducing the updates like @freshgiammi PR * test(react-router): route context breaking for layout routes * test(react-router): cases testing route context in layout routes for `beforeLoad`, `loader`, and `useRouteContext` * chore: use the useRouteContext hook from the route definition * chore: mistake in the test * test: wrap the render calls in `act` * test: wrap createRouter in `act` * test: remove magic sleeps * test: nested routes * test: loaders * test: failing tests for the loaders * test: react strictmode for loaders * test: dont use `waitFor` rather wait for elements in the DOM * test: more async-work delayed checking of route context * test: add an invalidate call after * fix: remove the recursive `this.load()` call The patch implemented by @freshgiammi in #1662 Co-Authored-By: Gianmarco Rengucci <16855748+freshgiammi@users.noreply.github.com> * refactor: restore the `checkLatest()` call here * test: remove these tests for routeContext in redirects --------- Co-authored-by: Gianmarco Rengucci <16855748+freshgiammi@users.noreply.github.com>
Possibly fixes #1650