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

use dynamic-cdn-webpack-plugin for react and react-dom #3914

Closed
wants to merge 1 commit into from
Closed

use dynamic-cdn-webpack-plugin for react and react-dom #3914

wants to merge 1 commit into from

Conversation

swyxio
Copy link
Contributor

@swyxio swyxio commented Jan 24, 2018

Hi

in connection with the community suggestion here: #2758

this is how it would work in react-scripts. i have only implemented this for production as I think it's probably not needed (possibly might cause some confusion if developing react at the same time) in development.

This PR results in the CRA bundle size going from 36 kb:

image

to 6kb (pardon the edited App.js text, i did not commit that lol):

image

and TTI should improve in actual deployed situation due to parallelized loading (not to mention caching benefits).

I have erred on the side of verbose with the comments for CRA users who may be unfamiliar with this plugin but am open to taking them out.

@swyxio
Copy link
Contributor Author

swyxio commented Jan 24, 2018

ummm appveyor is failing? i see its a source of a lot of anguish #2628 #2400

@@ -376,6 +377,11 @@ module.exports = {
minifyURLs: true,
},
}),
new DynamicCdnWebpackPlugin({
only: ['react', 'react-dom'],
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work when the app is cached using service worker and the network is offline? IMO this needs to be opt-in if we want to move forward with it

Copy link
Contributor Author

@swyxio swyxio Jan 24, 2018

Choose a reason for hiding this comment

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

this is a decision the maintainers should take. we already discussed the key motivations in #2758. @gaearon and others are in favor of using CDNs in general, but I understand your concern.

as far as SW and CRA are concerned I know we are moving towards making them opt-in: #2554. its an easier call there because its just commenting out one line of code. but still. we're moving away from it for now.

IMO CRA should cater to the 90% use case as we always have eject as an option. thats what its for. as far as i understand it, service workers are not in the 90% use case for CRA (this is why we keep getting so many issues about it)

as a last resort: if we really want to we can check for something like process.env.CDNModules to opt out of it for build.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea but if people chose to opt-in for SW, it broke their expectation because the created app is no longer working offline.. I think it should be a choice between cdn capable vs offline capable. Or maybe we can configure sw-precache to cache the files from cdn?

@iansu
Copy link
Contributor

iansu commented Jan 24, 2018

This is great but I agree with @viankakrisna that this functionality should be opt-in. I'm not sure about the best way to achieve that though.

@viankakrisna
Copy link
Contributor

maybe something like vendors.js from #3145? maybe name it cdn.js.

@chj-damon
Copy link

so can this feature be used now? @sw-yx

@swyxio
Copy link
Contributor Author

swyxio commented Feb 1, 2018

@chj-damon I've tested it and its usable, but @iansu and @viankakrisna want to do this through opt in instead of by default. so I'm not sure how they want to proceed.

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@gaearon gaearon closed this Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants