fix(router): context issues#1907
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 50f0b4a. 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. |
| location: next, | ||
| checkLatest: () => this.checkLatest(promise), | ||
| onReady: async () => { | ||
| // eslint-disable-next-line ts/require-await |
There was a problem hiding this comment.
since we are awaiting this, shouldn't this.startViewTransition then return a promise?
There was a problem hiding this comment.
eslint complains that there is no await 🤷
There was a problem hiding this comment.
if I remove the await and this disable comment, eslint does not complain
this currently fails!
AssertionError: expected "spy" to be called 2 times, but got 1 times
❯ tests/routeContext.test.tsx:417:18
415|
416| // Expect double call: once from preload, once from navigate
417| expect(mock).toHaveBeenCalledTimes(2)
| ^
418| })
419|
This comment was marked as resolved.
This comment was marked as resolved.
* chore(examples): cleanup the about route in "quickstart-file-based" * style(react-router): eslint on "createLazyRoute.test.tsx" * test(react-router): add a test to "createLazyRoute" to check that the heavy component is rendered in the DOM using testing-library * test(react-router): duplicate the preload test using screen rendering from testing library * test(react-router): forgot to add preload intent * chore(react-router): add the comment above the `runBeforeLoad` call stating that we're actually running it * chore(react-router): add comment above the `runLoader` call * chore(react-router): remove the destructuring of `p` * style(react-router): disable these eslint warnings in the `router.ts` file * chore(react-router): undo the `params: p` destucturing change
@tannerlinsley regarding the above mentioned. Is my assumption correct? If so, is this behavior expected (by design) or not really? |
It's partially correct. It should be rerendering any components that are subscribed to parts of the state that actually change. This is why we built the @tanstack/store library and the @tanstack/react-store adapter. It uses useSyncExternalStore and fine-grained selectors to determine if the "slice" that we return from the selector has changed (even shallowly compared). If it is, then it's rerendered. In the case that I just fixed, we were returning a slice in |
|
@tannerlinsley is there any checklist on what's remaining until this gets merged? Do you think it will be released by the next week? |
|
this needs to be fixed and of course test need to pass |
…ime of preload** This fixes a special case where you could hover over a link and start a preload, then click the link before it resolved. Before, any preload updates (including a successful state update) were ignored if the match was active, so in this case, the newly active match that hadn't resolved yet would never get updated since it is now active. This change only disallows updates to the original match IDs that are currently active.
|
@tannerlinsley this was probably a mistake so I added that |
|
Thank you 🕺 |
12 participants
It looks big, yes. But it boils down to:
runBeforeLoad, etcgetMatch()andupdateMatchexclusively, instead of reading and writing to a mix of local and state variables. This has already gotten rid of a few areas where we were definitely reading from stale data (but likely not getting any bugs reported)fixes #1892
fixes #1886
fixes #1752
fixes #1752
fixes #1827
fixes #1534
fixes #1777
fixes #1814
fixes #1656
fixes #1796
fixes #1774
fixes #1751
fixes #1882
fixes #1795
fixes #1551
fixes #1928
fixes #1899
fixes #1646
fixes #1929