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: Add swHasUpgrade but prevent auto reload. #517

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

richtera
Copy link
Contributor

Ticket ID

https://app.clickup.com/t/2645698/DEV-11966

Description

provide a new boolean called swHasUpgrade using provide('swHasUpgrade', hasUpgrade)
the value will be set if an upgrade is detected during the initial page load. The upgrade will be allowed,
but the new version will not be initialized such that we can notify the user they can reload to get an upgrade
next time. It should automatically upgrade on the next full navigation to another page anyways.

@Hugoo
Copy link
Contributor

Hugoo commented Oct 30, 2024

Task linked: DEV-11966 Remove additional page reload

Copy link
Contributor

github-actions bot commented Oct 30, 2024

Deployed with Cloudflare Pages ☁️ 🚀 🆗

app.vue Outdated
newWorker.addEventListener('statechange', () => {
if (newWorker.state === 'activated') {
// Notify the user that an update is available
console.log('A new version is available. Please refresh.')
Copy link
Contributor

Choose a reason for hiding this comment

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

from what I can see this is just logging on the console log and not really notifying the user. If he doesnt open the devtools he wouldn't know that he needs to refresh the page to get the update right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was for debugging. Removing.

Copy link
Member

@JeneaVranceanu JeneaVranceanu left a comment

Choose a reason for hiding this comment

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

LGTM

app.vue Outdated
})
const hasUpgrade = ref<boolean>(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's name it hasServiceWorkerUpgrade and move to the top where all refs sit.

Also qq, would that also work if we wrap the below code into function and call in onMounted hook as setupServiceWorker?

nuxt.config.ts Show resolved Hide resolved
nuxt.config.ts Outdated
Comment on lines 30 to 40
path: '~/node_modules/vue-instantsearch/src/components',
sourcemap: true,
}, // Enable source map generation for components
{
path: '~/node_modules/vite-plugin-vue-inspector/src',
sourcemap: true,
}, // Enable source map generation for components
{
path: '~/node_modules/@vite-pwa/nuxt/dist/runtime/components',
sourcemap: true,
}, // Enable source map generation for components
Copy link
Collaborator

Choose a reason for hiding this comment

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

qq, do we need the source maps for this node_modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok the sourcemaps are good to have, but it turns out nuxt prepare is reading this section and for example "pages" will crash because the route names are not defined and will by default be defined as the concat of the path names which ends up using symbol names like 404 (which is a number) or [profileAddress]BuyLyx which is obviously not a good symbol name. I removed pages and some of the other one. Each component that could possibly cause errors at runtime is nice to have because when an error occurs then sentry will show the actual source code rather than just minified javascript garbage. But obviously none of this is like "oh my god, there are no symbols" :)

nuxt.config.ts Outdated Show resolved Hide resolved
@dzbo
Copy link
Collaborator

dzbo commented Nov 1, 2024

@richtera we are getting there, can you check why lint is complaining?

@richtera
Copy link
Contributor Author

richtera commented Nov 1, 2024

@richtera we are getting there, can you check why lint is complaining?

See above comment about "pages"

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

Successfully merging this pull request may close these issues.

5 participants