Skip to content
New issue

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

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

Already on GitHub? # to your account

feat!: move client to @vite-pwa/workbox-window #634

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

userquin
Copy link
Member

@userquin userquin commented Dec 31, 2023

This PR also includes installing event support for VanillaJS and all frameworks:

  • VanillaJS: onInstalling and onUpdateFound callbacks
  • Frameworks: same callbacks for VanillaJS and reactive installingSW and updatingSW props

I need to review if installingSW and updatingSW can be read only (when exposing them), the logic should be handled internally, installing event is a transient event.

NOTE: make read-only in react/preact is almost imposible if we don't include some state management library, in svelte we'll need to add extra logic to handle writable stores (remove subcriptions), I have no idea what happens when returning only the solid accessor (the only we can use read-only is vue via readonly or computed from the ref).

NOTE: this PR should be included only in v1.0.0 major version.

/cc @antfu I removed ssr.noExternal from main plugin, @vite-pwa/workbox-window is ESM-first with dual ESM/CJS package exports.

closes #620
supersedes #623

Copy link

netlify bot commented Dec 31, 2023

Deploy Preview for vite-plugin-pwa-legacy ready!

Name Link
🔨 Latest commit ed83e56
🔍 Latest deploy log https://app.netlify.com/sites/vite-plugin-pwa-legacy/deploys/65c8f31c0cf5dd0008e6bdb6
😎 Deploy Preview https://deploy-preview-634--vite-plugin-pwa-legacy.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@userquin userquin changed the title feat: move to @vite-pwa/workbox-window feat!: move client to @vite-pwa/workbox-window Dec 31, 2023
@TheNoim
Copy link

TheNoim commented May 22, 2024

Hey, it would be great if at least one of the two PRs (this one or #623) could actually land. Is there a particular reason why both PRs are stalled?

If there are some major roadblocks, it would be helpful to at least expose the Workbox instance to install the events temporarily yourself.

@userquin
Copy link
Member Author

We need to do more tests, rn the logic is wrong, workbox-window logic in vite pwa is wrong (we reverted a commit a few months ago in pwa plugin), we are also awaiting the new Google team, no idea if workbox repo Will be finally deprecated (2 years)

@TheNoim
Copy link

TheNoim commented May 22, 2024

We need to do more tests, rn the logic is wrong, workbox-window logic in vite pwa is wrong (we reverted a commit a few months ago in pwa plugin), we are also awaiting the new Google team, no idea if workbox repo Will be finally deprecated (2 years)

Can we at least get #623 merged for now? The PR is smaller and would help for now.

@userquin
Copy link
Member Author

userquin commented May 22, 2024

The logic is not 100% correct, we have some "race" condition when using multiple clients (tabs or browser instances); the offlineReadyVal logic is wrong since that value is true when the sw is installed and activated. The problem is the missing installing event from workbox-window, it is NOT fired: these lines missing in original workbox-window https://github.com/vite-pwa/workbox-window-es/blob/main/src/Workbox.ts#L434-L441, I'm not sure if this is correct, requires more testing.

You can add the logic in your project if you want via onRegisteredSW.

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

Successfully merging this pull request may close these issues.

try to expose new version detected
2 participants