Reads CDN_URL to populate webpack publicPath #694
Conversation
|
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 - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
ghost
added
the
| @@ -25,14 +25,21 @@ if (env['process.env.NODE_ENV'] !== '"production"') { | |||
| } | |||
|
|
|||
| // We use "homepage" field to infer "public path" at which the app is served. | |||
| // You may also use the environment variable CDN_URL to set the homepage to an absolute url | |||
There was a problem hiding this comment.
This comment now looks a bit messy. Can you please format it so that sentences start with capital letters, end with newlines, and the complete comment doesn't look like it's written by two different people?
| @@ -64,7 +83,7 @@ module.exports = { | |||
| // containing code from all our entry points, and the Webpack runtime. | |||
| filename: 'static/js/bundle.js', | |||
| // In development, we always serve from the root. This makes config easier. | |||
There was a problem hiding this comment.
The change directly contradicts the comment above it. Please be sure to read the comments above the code you are changing, and keep them in sync.
| @@ -64,7 +83,7 @@ module.exports = { | |||
| // containing code from all our entry points, and the Webpack runtime. | |||
| filename: 'static/js/bundle.js', | |||
| // In development, we always serve from the root. This makes config easier. | |||
| publicPath: '/' | |||
| publicPath: publicPath | |||
There was a problem hiding this comment.
I don't understand why this change is necessary. This would break the development workflow because it would request files from CDN instead of the local server. Is this change in the development configuration intentional?
There was a problem hiding this comment.
This assumes people are going to serve both index.html and the js bundle at different hosts in dev just like they would do in production.
For most of my local dev I'm loading a react app into a rails view, (not using cra's generated html) during local dev and production. This makes that kind of thing possible without changing how it works for anyone not using $CDN_URL.
There was a problem hiding this comment.
Maybe CDN_URL should always be webpacks host + port, in dev?
There was a problem hiding this comment.
I think we should file a separate issue for discussing Rails development workflow. I’m not familiar with it so it’s not obvious to me how this works.
There was a problem hiding this comment.
I can remove this (removes most of the benefit I would get from this pr :( but I understand if it's not suitable here.)
Just want to point out that it's not rails specific really. It's the only way to load your react bundle into a host other than the same host that's serving the js files, Currently we are locked down to CRA serving the html.
There was a problem hiding this comment.
Let’s discuss more in the issue then so that I can get a better understanding of what you wanted. If you could guide me through your development workflow, what’s running on what port, and how you intend to use CDN_URL, etc, that would help.
| } else { | ||
| // no homepage | ||
| console.log('To override this, specify the ' + chalk.green('homepage') + ' in your ' + chalk.cyan('package.json') + '.'); | ||
| console.log(); | ||
| console.log('Alternatively, you may change the absolute URL for all files by setting a CDN_URL environment variable'); |
There was a problem hiding this comment.
I think these logs are too noisy. Let’s remove them as most users won’t need them. Those who need them will look for “CDN” in “Usage Guide”.
SeeThruHead
force-pushed
the
CDN_URL
branch
from
0a333c3
to
d51e615
Compare
SeeThruHead
force-pushed
the
CDN_URL
branch
from
d51e615
to
f5cc453
Compare
SeeThruHead
force-pushed
the
CDN_URL
branch
2 times, most recently
from
1dcb093
to
dc665bb
Compare
closes facebook#647 Previously we only allowed settings the pathname via the "homepage" key in package.json Allows user to configure the full base URL via a $CDN_URL env variable Development has the same behaviour.
SeeThruHead
force-pushed
the
CDN_URL
branch
from
dc665bb
to
768c9e8
Compare
|
#703 has landed. Can you please send a new PR on top of it that honors This would make it consistent with how the code already works (that’s the name of the variable we expose). Thanks! |
|
Hi there! Please test it and don't hesitate to reach out if this doesn't solve your specific use case! |
closes #647
Previously we only allowed settings the pathname via the "homepage" key
in package.json
Allows user to configure the full base URL via a $CDN_URL env
variable
Development has the same behaviour.