Nadav/handle route exports #1225
Conversation
…/handle-route-exports
There was a problem hiding this comment.
This isn't new to the code, but this PR introduces many additional listeners to the module system. I'm concerned that these listeners are not being properly disposed of, which could lead to accumulating observers on the module system and potential memory leaks.
| import { Outlet, useLoaderData } from '@remix-run/react'; | ||
| const loaderPromise = new Promise((resolve) => { | ||
| const globalListener = () => { | ||
| debugger; |
| if (newEmail && newFullName) { | ||
| userNames.set(nickname, { email: newEmail, fullName: newFullName }); | ||
| return json({ | ||
| message: existing ? 'User updated' : 'User created', |
There was a problem hiding this comment.
No test for "User updated" or "User does not exist"? maybe simplify the fixture?
|
|
||
| const importModuleAndUpdate: DynamicImport = (filePath, cb) => { | ||
| const { moduleResults, dispose } = requireModule(filePath, (newResults) => { | ||
| setHandle((newResults.results as { handle?: unknown }).handle); |
There was a problem hiding this comment.
Should be ok as the module system returns {} on error, but maybe the DynamicImport should be generic and the remix integration could expect it's own module interface.
There was a problem hiding this comment.
maybe but not in this PR
| setHandle((newResults.results as { handle?: unknown }).handle); | ||
| const linksFunction = (newResults.results as { links?: LinksFunction }).links; | ||
| if(linksFunction){ | ||
| setLinks(linksFunction); |
There was a problem hiding this comment.
Wouldn't the condition here prevent links from being removed?
There was a problem hiding this comment.
or added if it was not there to begin with, i need to somehow revoke the memo of the router when an export is added or removed ( relevant for all conditional route exports )
There was a problem hiding this comment.
actually, the manifest changes when the exportnames change, revoking the memo, adding a test
There was a problem hiding this comment.
added a test and fix
| return isSerializedResponse(res) ? deserializeResponse(res) : res; | ||
| const lastResults: { current: IResults<unknown> } = { current: { status: 'loading', results: null } }; | ||
| const { moduleResults } = importModuleAndUpdate(filePath, (updated) => { | ||
| lastResults.current = updated; |
There was a problem hiding this comment.
What handles an error in loader after the loader updates into an error state?
There was a problem hiding this comment.
i dont understand the question
There was a problem hiding this comment.
If we start with a valid page that has a loader and then it becomes invalid, then the lastResults reference the stale previous value. Maybe it doesn't matter.
…odux-core into nadav/handle-route-exports
No description provided.