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

Compared the resolved swSrc and swDest paths in InjectManfiest to ensure they're different #1495

Closed
dsych opened this issue May 24, 2018 · 4 comments
Labels
Bug An issue with our existing, production codebase. workbox-build workbox-cli
Milestone

Comments

@dsych
Copy link

dsych commented May 24, 2018

Welcome! Please use this template for reporting bugs or requesting features. For questions about using Workbox, the best place to ask is Stack Overflow, tagged with [workbox]: https://stackoverflow.com/questions/ask?tags=workbox

Library Affected:
workbox-sw, workbox-build, etc.

workbox-build

Browser & Platform:
E.g. Google Chrome v51.0.1 for Android, or "all browsers".

all browsers

Issue or Feature Request Description:
Your request here.

Whenever you are injecting precache routes into an existing sw, you should be able to save the results into the file with the same name and pathname.

Usecase:

  • Build your project with a bundler (e.g. parcel) using workbox-build to inject precache routes into a sw. The sw could import different libraries, hence you can' t just generate a new worker since it should contain imported modules.
  • Instead, you could specify swSrc and srDest to be the bundeled version of the sw, so that workbox-build replaces RegEx inside that bundeled version retaining all of the dependencies.
  • Instead of throwing

Service worker injection failed: Error: 'swSrc' and 'swDest' should not be set to the samefile. Please use a different file path for
'swDest'.

It could be turned into a warning.

When reporting bugs, please include relevant JavaScript Console logs and links to public URLs at which the issue could be reproduced.

@jeffposnick
Copy link
Contributor

The complication there is coming up with a valid RegExp that would work regardless of what the contents of the injected precache manifest are. (And also the fact that while this is something that makes sense for your use case, this is likely to cause developer confusion for other developers who accidentally sets them both to the same value.)

My personal inclination is not to make this change, but if there are other folks who are strongly in favor of it as an option, we could reconsider.

@dsych
Copy link
Author

dsych commented May 25, 2018

@jeffposnick the thing is, this already the behavior you can observe, meaning if you provide relative url for your swDest like so dist/sw.js and an absolute url for swSrc like /tmp/repo/dist/sw.js, workbox-build will treat these two as different paths, when in reality they are the same (given that you are running inside /tmp/repo).
However, that seems like more of bug, than a feature.

@jeffposnick jeffposnick changed the title Allow swSrc be the same as swDest for injectManifest Compared the resolved swSrc and swDest paths in InjectManfiest to ensure they're different Jun 4, 2019
@jeffposnick jeffposnick added Bug An issue with our existing, production codebase. workbox-cli and removed Feature Request labels Jun 4, 2019
@jeffposnick jeffposnick added this to the v5 milestone Jun 4, 2019
@jeffposnick
Copy link
Contributor

Revisiting this one—I'm going to implement a chance in Workbox v5 that will actually compare the fully resolved swSrc and swDest paths to confirm that they're unique, and fail the build if they're the same file.

We can't support the use case of supplying the same file for each.

@jeffposnick
Copy link
Contributor

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-build workbox-cli
Projects
None yet
Development

No branches or pull requests

2 participants