workbox icon indicating copy to clipboard operation
workbox copied to clipboard

Support matching URI-encoded requests against the precache manifest

Open weiwei-lin opened this issue 3 years ago • 3 comments

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/[email protected]",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.

weiwei-lin avatar Jun 01 '21 02:06 weiwei-lin

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.

jeffposnick avatar Jun 01 '21 19:06 jeffposnick

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.

weiwei-lin avatar Jun 02 '21 02:06 weiwei-lin

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:

https://github.com/GoogleChrome/workbox/blob/f6fb94a9d8b922fdcb758129d94e6d2a959f2167/packages/workbox-precaching/src/utils/generateURLVariations.ts#L24-L29

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 avatar Jun 02 '21 18:06 jeffposnick

I think you are right.

Upon further investigation. I think this might be an issue with html-webpack-plugin rather than with workbox. See https://github.com/jantimon/html-webpack-plugin/issues/1771

weiwei-lin avatar Nov 02 '22 04:11 weiwei-lin