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

Fix: Adding path normalization for service worker #1437

Closed

Conversation

rschristian
Copy link
Member

What kind of change does this PR introduce?

Bug fix

Did you add tests for your changes?

No, I did not. If this warrants a test I can of course write one up.

Summary

I needed to change the publicPath of an application (via config.output.publicPath = ... in my preact.config.js) when I noticed the SW import was not normalized. If my path was https://example.com/project, the service worker would error out in the console with the URL of https://example.com/projectsw.js. Simple normalization issue.

The function to normalize URLs was already written so all I did was wrap the usage of __webpack_public_path__ with that function.

Does this PR introduce a breaking change?

No breaking changes

@rschristian rschristian requested a review from a team as a code owner September 23, 2020 05:04
@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2020

🦋 Changeset detected

Latest commit: 6f7d229

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
preact-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rschristian rschristian changed the title fix: Correcting path normalization for service worker Fix: Correcting path normalization for service worker Sep 23, 2020
@rschristian rschristian changed the title Fix: Correcting path normalization for service worker Fix: Adding path normalization for service worker Sep 27, 2020
Copy link
Member

@prateekbh prateekbh left a comment

Choose a reason for hiding this comment

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

Whats the reasoning behind this change?

This wont work for most cases where __webpack_public_path__ is different from /.

  • A service worker from a different domain is not allowed to control your web app.
  • A service worker from a subpath would only control that subpath which is not the use case of preact-cli's SW

@rschristian
Copy link
Member Author

rschristian commented Nov 20, 2020

Give me a bit of rope here while I explain, certainly not an expert.

I was deploying a small site to GH pages. / does not work as a public path as it will try to import resources from https://rschristian.github.io/ rather than the correct https://rschristian.github.io/<project_name>. To correct this, I set the public path with config.output.publicPath = 'https://rschristian.github.io/<project_name>';.

The problem that then sprung up was the lack of a trailing /. All other resource paths are normalized and imported correctly, but the SW gets https://rschristian.github.io/<project_name>sw-esm.js. Obviously this doesn't work.

Certainly let me know if this just shouldn't be done this way or what. Certainly possible I found the worst solution to the issue.

I assume this falls under the "controlling a subpath is not the use case of preact-cli's SW", so perhaps this is just something that I should disable instead.

@prateekbh
Copy link
Member

@rschristian so sorry didn't mean to sound rude in my previous comment. apologies.
Github pages is one of the few use cases where this might make sense. I don't have strong preferences here but as i explained for the most part this would not work in a case with a different _webpack_public_path_.

i feel we need to have a customizable way of registering service worker, so that we build it just the way we do today and let users register it for a subpath.

@rschristian
Copy link
Member Author

rschristian commented Nov 21, 2020

@prateekbh No worries, I didn't think it was remotely rude. I was just trying to convey that this may very well be a hacky solution on top of a hacky solution. My knowledge in this area is super lacking.

Is letting users register a SW for a subpath something for this PR to fix, maybe something this PR can be merged into, or just something else entirely? While I do believe that I understand that SWs + use of _webpack_public_path_ causes issues, that issue and possibility for problems exists even without this PR. This just helps correct the path when it's missing the trailing /.

Edit: At least as far as I'm aware. Does this have some bigger impact I'm not seeing?

@rschristian
Copy link
Member Author

Whoops, accidentally recreated this in #1648

@rschristian rschristian closed this Feb 3, 2022
@rschristian rschristian deleted the fix/swPathNormalization branch May 17, 2022 16:24
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.

None yet

2 participants