fix: new_defineRouter #991
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| for (const skipId of skip) { | ||
| if (await canSkip(pathname, skipId)) { | ||
| delete entries[skipId]; | ||
| } | ||
| } |
There was a problem hiding this comment.
| for (const skipId of skip) { | |
| if (await canSkip(pathname, skipId)) { | |
| delete entries[skipId]; | |
| } | |
| } | |
| const toSkip = await Promise.all(skip.map((skipId) => canSkip(pathname, skipId)); | |
| for (const skipId of toSkip) { | |
| delete entries[skipId]; | |
| } |
There was a problem hiding this comment.
Do you mean filter instead of map?
There was a problem hiding this comment.
canSkip also waits only a microtask. If we were to improve it, there must be a better way. So, l'd leave it for now.
| if ( | ||
| Object.keys(elements).some((id) => id.startsWith(ROUTE_SLOT_ID_PREFIX)) | ||
| ) { | ||
| throw new Error('Element ID cannot start with "route:"'); | ||
| } |
There was a problem hiding this comment.
Is this check needed if you're moving to use routeElement also?
There was a problem hiding this comment.
This is needed to avoid conflict because we move to routeElement.
| pathname: string, | ||
| slotId: SlotId, | ||
| ): Promise<boolean> => { | ||
| const pathConfig = await getMyPathConfig(); |
There was a problem hiding this comment.
Actually, should canSkip take the result of getMyPathConfig() as a parameter? It seems unnecessary to await it for every pathname checked.
There was a problem hiding this comment.
it's possible. await just waits one microtask in this case, so I didn't care much.
There was a problem hiding this comment.
I don't disagree it's not ideal and there is room for improvement. Let's revisit it later as we will be rewriting this anyway.
| return processSkip({ | ||
| 'route:/nested/baz': ( | ||
| return { | ||
| routeElement: ( |
There was a problem hiding this comment.
nice! what made you change your mind on this?
There was a problem hiding this comment.
Do you mean moving skip handling from outside to inside? It's clearer abstraction.
The drawback is that now the user of new_defineRouter doesn't have "skip" information, they can't optimize based on it. (They have to return elements that will be skipped. So, theoretically, it's the waste.)
There was a problem hiding this comment.
thanks, ya that makes sense
No description provided.