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

[feat] application versioning and update detection support #3412

Merged
merged 40 commits into from
Feb 1, 2022

Conversation

pzerelles
Copy link
Contributor

@pzerelles pzerelles commented Jan 19, 2022

This PR will offer two options for navigation errors in the router. These errors occur for example if a new version of the site is deployed while being open in a browser. The JS files for pages might have changed and so has the filename. The previous page JS files, that the router still knows about, will give an import error 500 when trying to navigate to that page. For more detail on the problem, see #87.

The kit.browser.router configuration option in svelte.conf.js is now an object with enabled and onError properties.

enabled is just a boolean that replaces the old kit.browser.router boolean option.

onError has two possible values

  • "fail": will result in exactly the same behaviour as now - import error 500
  • "reload": location.href will be set to force a reload when navigation failed and the app version changed

The "reload" option should be enough for most users with the problem described. "fail" is currently the default option.

The mentioned app version is an automatically generated timestamp, that can be accessed with

import { version} from "$app/env"

and can also be overridden by setting the environment variable VITE_APP_VERSION to a custom version string.

There is also an updated store that can be imported with

import { updated } from "$app/stores".

The app version will be checked periodically in the background by fetching "_app/version.json" every 5 minutes. The store is set to true when the fetched version differs from the running version. The store has also a check function, that will immediately perform the check and return the result as a promise.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Jan 19, 2022

🦋 Changeset detected

Latest commit: ede1b40

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 19, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: ede1b40

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

@benmccann
Copy link
Member

"redirect": location.href will be set to force a reload when navigation failed

I didn't look at the code, but just from the description, it sounds like "reload" would be a better name. That being said, I think that would rarely be the best behavior and not something I'd want to encourage, so I'd be much happier to remove this option and have the user implement it themselves (e.g. via the function)

@pzerelles
Copy link
Contributor Author

pzerelles commented Jan 20, 2022

That being said, I think that would rarely be the best behavior and not something I'd want to encourage, so I'd be much happier to remove this option and have the user implement it themselves (e.g. via the function)

The implementation of "reload" is trivial, so that's fine for me. I removed the option.

@Rich-Harris
Copy link
Member

Thank you! Personally I disagree about reloading not being the best option — I think it's exactly what you want to happen if you attempt to click a link (or goto) and the module can't be imported (unless it's because you're offline, in which case rendering the error page is appropriate). I'm not sure what other behaviour you'd want to implement?

The trouble, of course, is that the error might be happening for some other reason than a new deploy — perhaps the imported module immediately throws an error in the browser, or something. If we reload indiscriminately then we miss the opportunity to discover those cases. Instead, we need a cohesive idea of an app 'version' (that can be used to disable client-side routing pre-emptively, or show a toast, or assess whether API requests are up to date, etc) that is used to determine whether reloading is appropriate.

Stringifying functions is a no-go, by the way — it gets horribly confusing when suddenly closures don't work, etc. If we need that level of flexibility (I'm not sure we do, yet) then we need client-side hooks.

@pzerelles
Copy link
Contributor Author

pzerelles commented Jan 31, 2022

...we need a cohesive idea of an app 'version' (that can be used to disable client-side routing pre-emptively, or show a toast, or assess whether API requests are up to date, etc) that is used to determine whether reloading is appropriate.

I added an app version. The reload will only happen on error when the version changed.

Stringifying functions is a no-go, by the way — it gets horribly confusing when suddenly closures don't work, etc. If we
need that level of flexibility (I'm not sure we do, yet) then we need client-side hooks.

Understood, I removed the custom function option.

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Thinking on this further, I'm reaching the conclusion that there's no reason for any behaviour other than automatic reloading. This is an area where the framework needs to be a little bit opinionated and do the right thing, since most developers don't really think about long session handling until they get bitten. Since client-side routing is best thought of as a progressive enhancement, falling back to a full page refresh should be perfectly acceptable in a well-written app.

To that end, I think we should get rid of the onError option (though if there's a compelling reason not to, I would be interested to hear it).

As mentioned inline, we need to be able to configure both the version string and the polling interval. I propose the following:

// svelte.config.js
export default {
  kit: {
    version: {
      name: Date.now(), // default to timestamp
      pollInterval: 60_000 // default to checking once a minute
    }
  }
};

packages/kit/src/core/build/build_client.js Outdated Show resolved Hide resolved
packages/kit/src/core/build/build_client.js Outdated Show resolved Hide resolved
packages/kit/src/core/build/build_client.js Outdated Show resolved Hide resolved
packages/kit/src/runtime/client/renderer.js Outdated Show resolved Hide resolved
packages/kit/types/internal.d.ts Outdated Show resolved Hide resolved
packages/kit/src/runtime/client/renderer.js Outdated Show resolved Hide resolved
@pzerelles
Copy link
Contributor Author

Thank you. Thinking on this further, I'm reaching the conclusion that there's no reason for any behaviour other than automatic reloading. This is an area where the framework needs to be a little bit opinionated and do the right thing, since most developers don't really think about long session handling until they get bitten. Since client-side routing is best thought of as a progressive enhancement, falling back to a full page refresh should be perfectly acceptable in a well-written app.

To that end, I think we should get rid of the onError option (though if there's a compelling reason not to, I would be interested to hear it).

I totally agree, but I read the discussion on #87 and many people requested more configurability. For me personally, the reload is all I need and using SvelteKit in 4 projects already, that solution has helped with the problems after deploy. I don't know how the final decision on this is made and if I should remove the option right away or wait.

@Rich-Harris
Copy link
Member

We did it! Thanks so much for this mammoth effort @pz-mxu — this is a great addition to the framework

@pzerelles
Copy link
Contributor Author

Happy I could be of help.

@jycouet
Copy link
Contributor

jycouet commented Mar 14, 2022

I'm not sure it's the right way to comment here, but I'll give it a try.

I'm precisely looking at kit.browser.router onError reload (Because I'm deploying a lot atm 🥳)
But on SvelteKit v1.0.0-next.295 (latest now) I can't figure out how to have this excellent reloading behavior.

Could you help me?

@Antonio-Bennett
Copy link

Antonio-Bennett commented Mar 15, 2022

Hey @jycouet After reading through this seems like it is a configuration on svelte.config.js specifically https://kit.svelte.dev/docs/configuration#version. pollInterval is set as 0 by default so you need to change that to a non-zero value. Let me know if that helps :)

@jycouet
Copy link
Contributor

jycouet commented Mar 15, 2022

I did change pollInterval to 5000 to see.
But it's not doing onError => reload. Actually, as I'm not using it (updated), it's not doing anything 😅

Now, using updated I'm deploying to see if it's better.
In __layout.svelte

import { updated } from '$app/stores';

updated.subscribe(async (value) => {
  if (value) {
    // Hard refresh when version is not matching. 
    // I can test only on prod! (dev or preview, this is not working) https://github.com/sveltejs/kit/issues/3957
    // Todo:
    // Nicer solution here one day: https://github.com/sveltejs/kit/issues/3666
    // or move to web workers? https://github.com/sveltejs/kit/issues/3667
    window.location.reload();
  }
});

=> It didn't refresh my page on the next deployment! I didn't see an error as well... 🤔

It's really hard to debug as I don't know how to reproduce locally.
Do you have ideas?

@frederikhors
Copy link
Contributor

It didn't refresh my page on the next deployment!

Still not working?

@jycouet
Copy link
Contributor

jycouet commented Mar 15, 2022

Seems like it's not working :(

@frederikhors
Copy link
Contributor

I'm on 1.0.0-next.295 and updated store works. I tried with 5000 ms too...

@jycouet
Copy link
Contributor

jycouet commented Mar 15, 2022

🥳🥳🥳 I found out!!!

You need: NODE_ENV: production (I had an issue in my CI that this fixed now 👌).
Another excellent info, the version can be found in: VITE_SVELTEKIT_APP_VERSION

@EskelCz
Copy link

EskelCz commented Sep 30, 2023

@Rich-Harris Isn't there an issue of the app losing its state? So for example a long text the user has written. I would rather show a warning message that the app was meanwhile updated and ask the user to refresh when it's convenient to them.

Also I love that SvelteKit at least has a solution, but when using Svelte+Vite only, there's no solution to be found. :( Maybe I'm missing something. This is the issue thread there: vitejs/vite#11804

Thanks for your consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants