Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[breaking] move hydrate and router options to handle #3397

Closed
wants to merge 16 commits into from
Closed

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented Jan 18, 2022

It doesn't really make sense to put these options in the .svelte files because then they get shipped to the client where they're not needed (especially in the case of hydrate), so they make more sense as resolve options

On the client-side, you can implement router: false on a specific link using rel="external". I'm not sure I've ever seen a use case for doing it to all links on a page besides just saving the weight of the JS router, which you can still do with the resolve option. It's possible we could add a method alongside cancel if there were such a use case

For now this makes hydrate and router booleans, but we could easily allow people to continue specifying these options in the page if we made them functions which took the page as a parameter. I think this could be a good idea because prerender really doesn't need to be shipped to the client either, since it only matters during build-time. I'd like to make it a resolve option as well, but fear that may be more disruptive if backwards compatibility is not kept. Sadly it's not tree shaken as unused today

The one possible alternative to this PR for getting stuff out of the client would be to implement something like <script context="module:ssr"> as suggested in sveltejs/rfcs#27

hydrate remains as an app-level option because it is passed to the compiler

@changeset-bot
Copy link

changeset-bot bot commented Jan 18, 2022

🦋 Changeset detected

Latest commit: 492f169

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jan 18, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 492f169

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61e7101640ad480008402c7a

@Conduitry
Copy link
Member

Do we still have the ability to disable all JS on the page? Would that be done just by setting hydrate = false?

@benmccann
Copy link
Member Author

benmccann commented Jan 18, 2022

You would do it with the resolve options now instead of the page options

@benmccann benmccann changed the title [breaking] remove per-page router option [breaking] move hydrate and router options to handle Jan 18, 2022
@geoffrich
Copy link
Member

The demo app will need to be updated too:

// we don't need any JS on this page, though we'll load
// it in dev so that we get hot module replacement...
export const hydrate = dev;
// ...but if the client-side router is already loaded
// (i.e. we came here from elsewhere in the app), use it
export const router = browser;

@Rich-Harris
Copy link
Member

The reason we removed export const ssr was that in the cases where you wanted to make it false, you couldn't load the page in order to know the value of the property. Moving it to resolve was a marked reduction in usability, but the trade-off made sense because otherwise it basically didn't work at all.

Here, the trade-off is a bit different — we're sacrificing ergonomics to save a very small number of bytes. If we added the function form, we'd be adding the bytes right back. To me it really doesn't seem worth it.

I'm not sure I've ever seen a use case for doing it to all links on a page besides just saving the weight of the JS router

I have — this covid tracker embed on the front page of https://www.nytimes.com is rendered with SvelteKit — it's part of the same app that powers the tracker itself. Without disabling the router, clicking on a link will cause bad things to happen, since we're on a page that Kit doesn't 'own':

image

It's not practical to put rel="external" on all the links in the embed, because some of the components might be reused by other parts of the app. This is admittedly a niche scenario, but it demonstrates by controlling router independently of hydrate can be useful.

If we were really motivated to remove those bytes, one option could be to add __config.json files alongside pages, or something like that. It does add a bit of noise and complexity though.

@benmccann
Copy link
Member Author

I don't feel very strongly about this, so won't object to closing it. Alternatively, maybe we can just remove the page-level router flag add a method alongside cancel that would let you decide to do the navigation as a server-side navigation instead of a client-side navigation? I think it'd be more powerful and would let us remove all the router enable/disable stuff

a very small number of bytes

It's not just the bytes for the flag, but also for the ability to enable and disable the router, which I'm not sure is strictly necessary and adds a bit more complication

I have — this covid tracker embed on the front page of https://www.nytimes.com

I'm assuming it would disable the router app-wide though in that case and not just on the one page? Because you could still do that here

@Rich-Harris
Copy link
Member

Alternatively, maybe we can just remove the page-level router flag add a method alongside cancel

That would only work if hydrate was true

It's not just the bytes for the flag, but also for the ability to enable and disable the router

I could probably be persuaded that disabling the router when you navigate to a router = false page is unnecessary. It's really not a lot of code there — three this.enabled references in if conditions and two methods that do nothing beyond toggle the boolean

I'm assuming it would disable the router app-wide though in that case and not just on the one page?

The router is enabled on the app as a whole, but the embeds have router = false

@benmccann benmccann closed this Jan 21, 2022
@benmccann benmccann deleted the rm-router branch January 21, 2022 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants