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

Support matching URI-encoded requests against the precache manifest #2856

Closed
weiwei-lin opened this issue Jun 1, 2021 · 5 comments
Closed
Labels
Bug An issue with our existing, production codebase. workbox-precaching

Comments

@weiwei-lin
Copy link

weiwei-lin commented Jun 1, 2021

Library Affected:
workbox-webpack-plugin

Browser & Platform:
all browsers

Issue or Feature Request Description:
the generated service worker doesn't not encode file names of the cached files.
For instance, the plugin will generate the following code.

e.precacheAndRoute([
    ...
    {url:"/ui/immutable/npm.@material.23acbb8b21aa4990d57d.bundle.js",revision:null},
    ...
]

However, when webpack-html-plugin will generate the following html.

...
<script defer="defer" src="/ui/immutable/npm.%40material.23acbb8b21aa4990d57d.bundle.js"></script>
...

Note that the @ is encoded to %40.

As a result, the file is not cached by the service worker and the page has to hit the web server to retrieve that file. This can be problematic when a new version is released and the original file no longer exist on the server. The old HTML file may still be served by the service worker, and the page may run into 404 when trying to get the dependencies.

I believe encoding the URL is the correct implementation. So we should fix it on the workbox side.

@jeffposnick
Copy link
Contributor

jeffposnick commented Jun 1, 2021

Hmm, I'm not really sure that encoding the @ in URLs automatically is the right thing for Workbox to do. My understanding is that URLs with unencoded @ in them are widely supported by browsers, and introducing a breaking change is something we'd like to avoid. (I also think that there would be issues around, e.g., whether to encode a as %20 or +, since both are valid.)

What you could do is use the urlManipulation option that can be passed in to precacheAndRoute() to tell workbox-precaching which URLs it should consider a precaching match. That function could decode any entities in the incoming URL and return that as a potential match.

Using this does require that you switch to the InjectManifest mode of the workbox-webpack-plugin instead of GenerateSW, since you need more control over the source of your service worker.

// Inside your swSrc file:
import {precacheAndRoute} from 'workbox-precaching';

precacheAndRoute(self.__WB_MANIFEST, {
  urlManipulation: ({url}) => {
    const decodedURL = decodeURIComponent(url);
    return [decodedURL];
  }
});

If that doesn't work for you, let us know and we can revisit.

@weiwei-lin
Copy link
Author

I fixed it with npm-patch. So strictly speaking, this is not blocking me.

I think your concern about backward compatibility makes sense. However, given @ is a reserved character in
https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
Is it possible that we at least add some documentation or show a warning when @ is in the precached filename?
A lot of npm packages are prefixed with @ and webpack-html-plugin has 6.6m download, I reckon many users will run into this issue. They probably just didn't realise it because the page is only broken when a dependency with @ is updated.

@jeffposnick
Copy link
Contributor

Thinking more about it, it's probably "safe" (from a semver perspective) if we added something like that custom urlManipulation function that called decodeURIComponent() to run as part of the default logic in workbox-precaching that tries to find equivalent cache entries for an incoming URL:

export function* generateURLVariations(url: string, {
ignoreURLParametersMatching = [/^utm_/, /^fbclid$/],
directoryIndex = 'index.html',
cleanURLs = true,
urlManipulation,
}: PrecacheRouteOptions = {}) {

So maybe we can just do that (assuming the logic shared above works; we'd add in a test case to confirm) and we wouldn't need to warn folks to begin with—it would just start working as expected.

@jeffposnick jeffposnick reopened this Jun 2, 2021
@jeffposnick jeffposnick added Bug An issue with our existing, production codebase. and removed workbox-webpack-plugin labels Jun 2, 2021
@jeffposnick jeffposnick changed the title File names are not encoded in the URL in the prefetch config. Support matching URI-encoded requests against the precache manifest Jun 2, 2021
@weiwei-lin
Copy link
Author

I think you are right.

Upon further investigation. I think this might be an issue with html-webpack-plugin rather than with workbox.
See jantimon/html-webpack-plugin#1771

@tomayac
Copy link
Member

tomayac commented Apr 25, 2024

Hi there,

Workbox is moving to a new engineering team within Google. As part of this move, we're declaring a partial bug bankruptcy to allow the new team to start fresh. We realize this isn't optimal, but realistically, this is the only way we see it working. For transparency, here're the criteria we applied:

Thanks, and we hope for your understanding!
The Workbox team

@tomayac tomayac closed this as completed Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An issue with our existing, production codebase. workbox-precaching
Projects
None yet
Development

No branches or pull requests

3 participants