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

Client public path #740

Closed
wants to merge 10 commits into from
Closed

Client public path #740

wants to merge 10 commits into from

Conversation

gregmartyn
Copy link
Contributor

@gregmartyn gregmartyn commented Aug 30, 2018

Adds a CLIENT_PUBLIC_PATH env var so that the dev environment can work on a non-http://localhost:3000 url.

E.g. I have a local setup somewhat similar to Laravel Valet https://laravel.com/docs/5.6/valet. We deal with many clients, so it's nice to be able to go to http://clientA.dev and http://clientB.dev instead of remembering which localhost port each client is on, or bringing one down before switching to the next.

Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

Generally speaking, environment variables should not have their environment name in them. I think the code for this PR is okay, just need to rework names of these options.

* `process.env.PORT`: default is `3000`, unless changed
* `process.env.HOST`: default is `0.0.0.0`
* `process.env.PORT`: The `BUILD_TARGET=server` build listens on this port for all NODE_ENVs. default is `3000`
* `process.env.PORT_DEV`: The `NODE_ENV=development` `BUILD_TARGET=client` listens on this port. Unused when `NODE_ENV=production`. default is `3001`
Copy link
Owner

Choose a reason for hiding this comment

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

Let’s discuss the naming convention here. Im not fan of PORT_DEV. Should probably be like HMR_PORT or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking back, I didn't even use that var and assumed PORT+1. I'll remove it unless there's any good reason to have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone else added PORT_DEV
f979863#diff-7c88c5c3855db79976ff110d7e59c9fc
Looks like I was carrying that over from packages/razzle/README.md

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm must have missed that. We should probably rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a different PR though, right?

@@ -236,17 +236,19 @@ export default Component;

**The following environment variables are embedded during the build time.**

* `process.env.RAZZLE_PUBLIC_DIR`: Path to the public directory.
* `process.env.RAZZLE_PUBLIC_DIR`: Absolute path to the public directory in the server's filesystem.
Copy link
Owner

Choose a reason for hiding this comment

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

Pretty sure it can be relative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It technically can, but the default is a path.resolve-generated absolute path. I figure it's better that it predictably be an absolute path, and anyone putting in a relative path can do so at their own risk. I changed the wording in the doc because "Path" alone didn't tell me to expect the default to be "/public" or "/full/path/to/public/"

* `process.env.RAZZLE_ASSETS_MANIFEST`: Path to a file containing compiled asset outputs
* `process.env.REACT_BUNDLE_PATH`: Relative path to where React will be bundled during development. Unless you are modifying the output path of your webpack config, you can safely ignore this. This path is used by `react-error-overlay` and webpack to power up the fancy runtime error iframe. For example, if you are using common chunks and an extra entry to create a vendor bundle with stuff like react, react-dom, react-router, etc. called `vendor.js`, and you've changed webpack's output to `[name].js` in development, you'd want to set this environment variable to `/static/js/vendor.js`. If you do not make this change, nothing bad will happen, you will simply not get the cool error overlay when there are runtime errors. You'll just see them in the console. Note: This does not impact production bundling.
* `process.env.VERBOSE`: default is false, setting this to true will not clear the console when you make edits in development (useful for debugging).
* `process.env.PORT`: default is `3000`, unless changed
* `process.env.HOST`: default is `0.0.0.0`
* `process.env.PORT`: The `BUILD_TARGET=server` build listens on this port for all NODE_ENVs. default is `3000`
Copy link
Owner

Choose a reason for hiding this comment

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

Isn’t this available on client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends what you mean by client. I'm talking about the webpack build target here. The browser connects to the PORT used by BUILD_TARGET=server for the HTML, then connects to PORT+1 for HMR. (HMR is the BUILD_TARGET=client)

README.md Outdated
* `process.env.NODE_ENV`: `'development'` or `'production'`
* `process.env.BUILD_TARGET`: either `'client'` or `'server'`
* `process.env.PUBLIC_PATH`: Only in used in `razzle build`. You can alter the `webpack.config.output.publicPath` of the client assets (bundle, css, and images). This is useful if you plan to serve your assets from a CDN. Make sure to _include_ a trailing slash (e.g. `PUBLIC_PATH=https://cdn.example.com/`). If you are using React and altering the public path, make sure to also [include the `crossorigin` attribute](https://reactjs.org/docs/cdn-links.html#why-the-crossorigin-attribute) on your `<script>` tag in `src/server.js`.
* `process.env.PUBLIC_PATH`: Only in used in `razzle build`. You can alter the `webpack.config.output.publicPath` of the client assets (bundle, css, and images). This is useful if you plan to serve your assets from a CDN. Make sure to *include* a trailing slash (e.g. `PUBLIC_PATH=https://cdn.example.com/`). If you are using React and altering the public path, make sure to also [include the `crossorigin` attribute](https://reactjs.org/docs/installation.html#using-a-cdn) on your `<script>` tag in `src/server.js`.
* `process.env.CLIENT_PUBLIC_PATH`: The `NODE_ENV=development` build's `BUILD_TARGET=client` has a different `PUBLIC_PATH` than `BUILD_TARGET=server`. Default is `http://${process.env.HOST}:${process.env.PORT_DEV}/`
Copy link
Owner

Choose a reason for hiding this comment

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

I don’t understand why this is needed. It’s just the public path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two different public paths that the browser connects to when in NODE_ENV=development. One for the webpack server build at PUBLIC_PATH, and one for the webpack client build. Razzle doesn't currently let you specify a PUBLIC_PATH for the client build, and assumes that it's available at http://${dotenv.raw.HOST}:${devServerPort}/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOST is the IP that the dev server is binding to, which may not be accessible from the host OS when the app is running inside a container/vm. We need some way to differentiate between what the user's browser connects to for HMR and what the dev server binds to, because when containers enter the picture, those can be two different things.

Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

Generally speaking, environment variables should not have their environment name in them. I think the code for this PR is okay, just need to rework names of these options.

Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

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

This looks fine. Just Go back to original readme aside from the addition of the new environment variable

@@ -246,7 +246,7 @@ export default Component;
* `process.env.NODE_ENV`: `'development'` or `'production'`
* `process.env.BUILD_TARGET`: either `'client'` or `'server'`
* `process.env.PUBLIC_PATH`: Only in used in `razzle build`. You can alter the `webpack.config.output.publicPath` of the client assets (bundle, css, and images). This is useful if you plan to serve your assets from a CDN. Make sure to *include* a trailing slash (e.g. `PUBLIC_PATH=https://cdn.example.com/`). If you are using React and altering the public path, make sure to also [include the `crossorigin` attribute](https://reactjs.org/docs/installation.html#using-a-cdn) on your `<script>` tag in `src/server.js`.
* `process.env.CLIENT_PUBLIC_PATH`: The `NODE_ENV=development` build's `BUILD_TARGET=client` has a different `PUBLIC_PATH` than `BUILD_TARGET=server`. Default is `http://${process.env.HOST}:${process.env.PORT_DEV}/`
* `process.env.CLIENT_PUBLIC_PATH`: The `NODE_ENV=development` build's `BUILD_TARGET=client` has a different `PUBLIC_PATH` than `BUILD_TARGET=server`. Default is `http://${process.env.HOST}:${process.env.PORT + 1}/`
Copy link
Owner

Choose a reason for hiding this comment

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

Can we actually remove all changes to the README aside from this line in this PR? Let's add the others separately.

@thecodedrift
Copy link

In case someone else finds this thread & you need this in Razzle <= 2.4.0, you can use the following in your razzle.config.js as a workaround.

module.exports = {
  modify: (config, { target, dev }, webpack) => {
    // note the use RAZZLE_ prefix
    if (dev && process.env.RAZZLE_CLIENT_PUBLIC_PATH) {
      config.output.publicPath = process.env.RAZZLE_CLIENT_PUBLIC_PATH;
    }
    return config;
  }
};

When you upgrade razzle, nothing will break, and you can migrate from RAZZLE_CLIENT_PUBLIC_PATH to CLIENT_PUBLIC_PATH at your leisure.

@gregmartyn gregmartyn deleted the client_public_path2 branch January 24, 2020 23:08
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.

4 participants