notebooks: Group notebook routes#47897
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits f219a35 and ccfa75c or learn more. Open explanation
|
philipp-spiess
left a comment
There was a problem hiding this comment.
I think this makes sense for now in the very short term but with React router 6 we'd actually like to move more and more routes into a shared router map that is global so we can actually properly do data preloading (e.g. can't preload notebook data early if we don't know which specific page it even needs).
Any way that you could use the routes.tsx file instead somehow in Sveltekit?
I guess I can just replicate whatever Layout is doing. Load the routes definition, pull out the ones I need and create routes for them. |
|
I can also discard this PR and create my own Notebook specific routing logic for the SvelteKit app. The Notebooks subtree is just easier to test than insights or batch changes and it seemed to be a reasonable change. |
Thanks for communicating this, @philipp-spiess! It's not planned for the short-term future because we're focused on top-level routes and the application shell at the moment, but eventually, we want to lift most of the routes to the top level. This means there won't be a standalone router for the notebooks section. |
valerybugakov
left a comment
There was a problem hiding this comment.
Commenting to remove from the review queue.
0ad2195 to
4a6d460
Compare
08938b4 to
f219a35
Compare
This PR consolidates notebook routes into a single component.
I hope that doesn't interfere with the React router migration efforts. It makes it easier to integrate notebooks with the SvelteKit app and is also more in line with how other product areas are structured (e.g. insights, code monitors, ...).
This also removes the use of
showSearchNotebooks, which has been removed in https://github.com/sourcegraph/sourcegraph/pull/46086.(note: Adding people as reviewers who have better knowledge about react router than me)
Test plan
/notebooks/newredirects to the notebooks list pageApp preview:
Check out the client app preview documentation to learn more.