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

adding flag to disable service worker #377

Merged
merged 2 commits into from
Sep 26, 2017
Merged

adding flag to disable service worker #377

merged 2 commits into from
Sep 26, 2017

Conversation

prateekbh
Copy link
Member

fixes #371

Copy link
Member

@reznord reznord left a comment

Choose a reason for hiding this comment

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

LGTM 👍

This PR can be merged once we get the webpack3 in.

@@ -199,7 +199,12 @@ const production = config => addPlugins([
/push-manifest\.json$/,
/.DS_Store/
]
})
}),
new webpack.DefinePlugin({
Copy link
Member

Choose a reason for hiding this comment

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

I had posted in chat about this but I'll echo here for visibility - does the object form here replace as optimally as this?

new webpack.DefinePlugin({
  'process.env.ADD_SW': config.serviceWorker
});

Copy link
Member

Choose a reason for hiding this comment

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

I believe yargs will have camelCased the flag name at this point.

@prateekbh Have you checked/ran this? This was my thought as well.

@prateekbh
Copy link
Member Author

@developit @lukeed it worked for me bothways with and without camel casing

@prateekbh
Copy link
Member Author

btw made suggested changes, is it ready to merge yet?

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

LGTM!

@reznord reznord added this to the 2.0 milestone Sep 26, 2017
@reznord reznord merged commit 81c7bb2 into master Sep 26, 2017
@reznord reznord deleted the noServiceWorkerFile branch September 26, 2017 12:32
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.

How to unregister / remove any registered service workers?
4 participants