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

Added options to allow for overrides to workbox-webpack-plugin #5369

Closed
wants to merge 1 commit into from
Closed

Added options to allow for overrides to workbox-webpack-plugin #5369

wants to merge 1 commit into from

Conversation

mymattcarroll
Copy link

@mymattcarroll mymattcarroll commented Oct 9, 2018

Closes #5359

Verified by:

  1. Running yarn build in create-react-app root and ensuring service worker uses options return by workbox.config.js

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@mymattcarroll
Copy link
Author

Still a WIP at the moment, have not verified the changes are working yet. Just looking for some validation on approach right now.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mymattcarroll
Copy link
Author

@Timer, I not sure what has caused the AppVeyor build to fail?

I can see in the logs that a file is being imported from outside of the ./src directory:

You attempted to import ../sample which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.

However, I can see that same error message in the Travis build and it is passing?

@Nick-t-go
Copy link

Been looking for this update... So confused how workbox was added b/c it is deemed the superior way to configure a PWA, but we have no ability to edit the Workbox webpack plugin that is being used, or do we?

@thelastnode
Copy link

thelastnode commented Nov 6, 2018

This is great, this is the final piece (set skipWaiting: true) that would allow us to move from react-scripts-ts to vanilla create-react-app!

Is there anything pending on this PR that I can help with? Would love to have this merged for the next release.

@seanmheff
Copy link

Looking forward to this getting merged. I am also looking forward to being able to set the skipWaiting: true option.

@imranismail
Copy link

This is the only show stopper for us as well. Looking forward to this merge. Can't stress how important skipWaiting is for UX.

@dstapleton92
Copy link

Really looking forward to this change as well! What's the status here? Does it need anymore work? If so, I'd be happy to help pitch in.

Does anyone know why the tests are failing?

@mymattcarroll
Copy link
Author

@dstapleton92, I have not had the time to investigate the tests that are failing, other than AppVeyor seems to be failing on master last time I checked. I would appreciate any assistance you can supply 😄.

@Timer Timer added this to the 2.x milestone Dec 6, 2018
@dstapleton92
Copy link

@mymattcarroll Yeah I see that. Doesn't look like it's from a change you introduced. Anyone have any ideas?

@mymattcarroll
Copy link
Author

@dstapleton92 seems a merge with the latest master code has fixed the broken tests. :)

@Timer any chance you could take another look at this?

@assimelha
Copy link

What's the status on this ? do you need any help merging this ?

@mymattcarroll
Copy link
Author

@RxAssim

What's the status on this ? do you need any help merging this ?

I assume that question was not for me?

@dstapleton92
Copy link

@mymattcarroll @Timer @iansu Does this look good to merge? My team would really like to re-enable the service worker for our application, but that's a no-go until we can configure it.

@iansu iansu modified the milestones: 2.x, 3.0 Mar 10, 2019
@mymattcarroll
Copy link
Author

I don't seem to have access to re-run the tests? Could someone with access try them again?

@johnpangalos
Copy link

You could always push a dummy file then remove, that should trigger the tests...

@johnpangalos
Copy link

So I've been unfortunately able to reproduce this error, going to see if we can fix it...

@oallouch
Copy link

oallouch commented Oct 2, 2019

Hi guys,

Sorry to ask you this, but do you have a potential release date ?
It's quite urgent for me and I feel like I'm gonna have to eject and I don't want to leave the comfort of the CRA nest.

Thx a lot in advance,

Olivier

@FezVrasta
Copy link
Contributor

@oallouch in the meantime you may use react-app-rewired + customize-cra, we have a dedicated adjustWorkbox plugin.

@rmoorman
Copy link

rmoorman commented Oct 2, 2019

@FezVrasta reading the source of the named customizer, it looks like this only allows to override constructor arguments for the GenerateSW Plugin.
Given the fact that GenerateSW is not the recommended plugin when it comes to further customization and using other APIs available to service workers (e.g. Web Push), but InjectManifest is, how about allowing for replacing the GenerateSW plugin entirely with InjectManifest?

Now regarding this PR, wouldn't it be better that it would be amended to allow for using the InjectManifest instead of the GenerateSW plugin?

@FezVrasta
Copy link
Contributor

@rmoorman feel free to send a PR to update the customizer! I wasn't aware of InjectManifest

@rmoorman
Copy link

rmoorman commented Oct 12, 2019

@FezVrasta if I can find any time to do so I can give it a try.

Besides that, that was not exactly my point. The problem I see with this, is that this (according to the workbox guys) does not easily allow to add custom logic and use browser apis such as web push as there still will be no way to use something else than GenerateSW out of the box. This PR does only account for passing other options to the GenerateSW plugin. Not for using another plugin altogether such as the recommended plugin allowing for custom logic in the service worker.

It is not useful in my opinion (even though I can totally agree with the desire to keep configurability to a minimum in default CRA) that a whole additional configuration file would be introduced just to set a few options on a plugin which should enable "service workers" in CRA but does not seem to be well suited to really get going with things service workers provide besides caching.

@marlonmleite
Copy link

I am sad to see initial solutions to solve problems of several people taking too long to be accepted.
Now I am unable to upgrade from 3.2.0 to 3.3.0 of the CRA, otherwise my react-app-rewired will stop working.
This PR solves problems and is always left out...
Since there are things that are just makeup for the CRA that are accepted faster than that.

@ianschmitz any progress to merge this or need help for this?

This partially meets, but with it it would be simpler to evolve to the solution of using InjectManifest conform @rmoorman :)

@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@marlonmleite
Copy link

@mymattcarroll @iansu @ianschmitz I opened PR #8485 and with it it will be possible to make any type of configuration of the Workbox, I was careful and left it without any break. It will be possible to use the most advanced features of the Workbox using InjectManifest, as GenerateSW is very superficial and limited.

@gerardcastell
Copy link

Does anyone know when is this feature going to be released? Thx!

@SoYoung210
Copy link

I want to override workbox config too, any update..? 👀

@ketys-from-meiro
Copy link

Any update on this? It could probably solve #7700 which causes blank pages in production builds as I described here: GoogleChrome/workbox#2299

Should be high prio, it's affecting clients, I always need to navigate them to wipe browsers cache and service worker. Other solution is to get rid of service worker, but it will not be possible to preserve app update procedure (modal window, update on click) and it can lead to BE/FE inconsistencies then if everyone has different app version due to cache.

@glebmikulko
Copy link

@ianschmitz @mrmckeb @petetnt Is there any news on this?

@9jaGuy
Copy link

9jaGuy commented Nov 13, 2020

#9205 switch workbox to InjectManifest plugin. The implementation is a bit more flexible, though it will not 100% the need for this but it gets us a step closer.

@jeffposnick
Copy link
Contributor

This should be closed, as it would revert the new, customizable InjectManifest behavior in c-r-a v4.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 3, 2021

Agreed, we can look at continuing to improve how we support service workers in the future - but the work that @jeffposnick did enables a lot of customisation.

@mrmckeb mrmckeb closed this Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for a workbox.config.js override file