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: use ManifestTransform when using @islands/pwa #144

Merged
merged 4 commits into from
Jul 1, 2022

Conversation

userquin
Copy link
Collaborator

@userquin userquin commented Jun 29, 2022

Description 📖

This PR uses workbox ManifestTransform when pwa is configured via @islands/pwa plugin/module: this way we can control the sw precache manifest, adding or removing entries.

Also includes the configuration based on prettyUrls on workbox.navigateFallback entry: will use the Vite base or / when not configured.

If the pwa plugin is configured via Vite instead using @islands/pwa plugin/module, using prettyUrls: false will produce an error on runtime when registering the sw, since it will have the html entries twice in the sw precache manifest: you have a comment about not adding the addRoutes entries on line 195 (the comment is about prettyUrls: true, since we'll end with the html file and also the route name, but registering the sw will work).

TODO:

  • the iles.config.ts docs file has a comment about excluding the html entries, we can remove it before merging this PR (the html entries on the globPatterns added, the new logic will remove all html entries and recalculate the revision for all routes provided in the SSG callback).
  • remove the console.log on the pwa module, I added a few logging and so you can check the result.

Background 📜

This was happening because I talk about it with ElMassimo ;)

The Fix 🔨

By changing the Vite PWA Plugin options to include the workbox ManifestTransform to regenerate the manifest entries after SSG build.

Screenshots 📷

Nopes

@nx-cloud
Copy link

nx-cloud bot commented Jun 29, 2022

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 218a868.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@userquin
Copy link
Collaborator Author

We need to talk about dynamic pages, should be excluded from the sw precache manifest: a lot of work to be done on server side and also including a custom sw (read antfu-collective/vitesse#169 and a simple implementation here antfu-collective/vitesse#252 (comment)).

@userquin
Copy link
Collaborator Author

@ElMassimo we can wait, we're releasing a few things on pwa repo, I'll update this PR when pwa 0.12.2 released (2/3 hours)

@ElMassimo
Copy link
Owner

Great, thanks Joaquin!

chore: remove skipWaiting and clientsClaim hack
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 30, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 218a868
Status: ✅  Deploy successful!
Preview URL: https://9b00d3fc.iles.pages.dev
Branch Preview URL: https://userquin-feat-pwa-use-pretty.iles.pages.dev

View logs

@userquin
Copy link
Collaborator Author

userquin commented Jun 30, 2022

@ElMassimo updated, I have another PR to add ESM support and fix some dev bug and: I think we should remove the support here adding the pwa plugin via vite.plugins entry

@ElMassimo
Copy link
Owner

Looks great! Throwing an error if the user provides the plugin in vite.plugins sounds good 👍

@userquin userquin marked this pull request as ready for review July 1, 2022 15:00
@userquin userquin requested a review from ElMassimo July 1, 2022 15:00
@userquin
Copy link
Collaborator Author

userquin commented Jul 1, 2022

we have to clean up some internal types of the plugin that are not needed anymore

@userquin userquin marked this pull request as draft July 1, 2022 15:51
@userquin
Copy link
Collaborator Author

userquin commented Jul 1, 2022

doing some cleanup

@userquin userquin marked this pull request as ready for review July 1, 2022 15:56
Copy link
Owner

@ElMassimo ElMassimo left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you @userquin 😃 !!

@ElMassimo ElMassimo changed the title feat(pwa): use ManifestTransform when using @islands/pwa feat: use ManifestTransform when using @islands/pwa Jul 1, 2022
@ElMassimo ElMassimo merged commit 84b641b into main Jul 1, 2022
@ElMassimo ElMassimo deleted the userquin/feat-pwa-use-pretty-urls branch July 1, 2022 18:08
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.

2 participants