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

"App updated" notification mechanism seems broken #8948

Closed
0gust1 opened this issue Feb 8, 2023 · 5 comments
Closed

"App updated" notification mechanism seems broken #8948

0gust1 opened this issue Feb 8, 2023 · 5 comments

Comments

@0gust1
Copy link
Contributor

0gust1 commented Feb 8, 2023

Describe the bug

In our app we use:

https://kit.svelte.d:ev/docs/modules#$app-stores-updated

and the following config:

version: {
      name: Date.now().toString(),
      pollInterval: 60 * 1000
    }

It worked quite well.

Recently we made an deps bump (sveltekit 1.0.11 => 1.3.9), and the bug started to show on our environments.

The bug:

the polling to APP_URL/_app/version.json works and the updated store keep being true. Our notification component telling the user to reload the app show itself at each poll.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-s85ex1?file=src/routes/+page.svelte

(Bug shows in npm run build && npm run preview mode or when deployed).

Locally, I reproduced it on a brand new sveltekit project created with npm create svelte@latest

Logs

No response

System Info

(not relevant IMHO, as we have also the bug in our cloud envs, which have a different OS/config)

  System:
    OS: macOS 13.1
    CPU: (8) arm64 Apple M1
    Memory: 52.05 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
    npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
  Browsers:
    Brave Browser: 109.1.47.186
    Chrome: 109.0.5414.119
    Firefox: 109.0.1
    Firefox Developer Edition: 102.0
    Safari: 16.2
  npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.0.0
    @sveltejs/kit: ^1.5.0 => 1.5.0
    svelte: ^3.54.0 => 3.55.1
    vite: ^4.0.0 => 4.1.1

Severity

blocking an upgrade

Additional Information

When I search in built/generates files, I find the timestamp returned by the url polled (APP_URL/_app/version.json) in output/client/_app/version.json

@eltigerchino
Copy link
Member

eltigerchino commented Feb 8, 2023

For now, you can remove the version.name setting in your config as the default value is already Date.now().toString().

The main cause of the issue is that multiple different date values are set during the build process, only when manually specifying version.name. It's calling Date.now() multiple times. Here's an updated repro showing the different version strings after build.

Maybe the fix is to call it once at the start of build and pass that string manually to different parts of the build process?

@Conduitry
Copy link
Member

I'm tending towards saying that, if you specify a version.name config, you're responsible for making sure it's consistent across multiple evaluations - whether it's coming from an environment variable, or you're looking up a git commit hash, or whatever. I don't really know what would be involved in making sure, within the framework, that we only ever evaluate the configuration once.

If that's the direction we go, that would make this a documentation issue.

@0gust1
Copy link
Contributor Author

0gust1 commented Feb 8, 2023

Thanks, beloved maintainers !

If that's the direction we go, that would make this a documentation issue.

Yes, I was wondering if we weren't a bit mislead by documentation. It states that default value is Date.now().toString(), so we thought we could give this config entry this value, without realizing the behavior underneath.

On our side, I think we're currently fine with deleting the version.name entry (anyway, IMHO a commit sha-1 would be cleaner for us, as we don't do environment specific builds).

0gust1 added a commit to 0gust1/kit that referenced this issue Feb 8, 2023
Doc update attempt, following sveltejs#8948
@0gust1 0gust1 mentioned this issue Feb 8, 2023
5 tasks
@0gust1
Copy link
Contributor Author

0gust1 commented Feb 8, 2023

@Conduitry made a small doc update attempt

Rich-Harris added a commit that referenced this issue Feb 9, 2023
* Update index.d.ts

Doc update attempt, following #8948

* Update index.d.ts

Better formulation

* Update packages/kit/types/index.d.ts

* changeset

---------

Co-authored-by: Rich Harris <hello@rich-harris.dev>
@Rich-Harris
Copy link
Member

closed via #8956

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

No branches or pull requests

4 participants