Handle browser arguments #7277
Handle browser arguments #7277
Conversation
arvigeus
requested review from
amyrlam,
bugzpodder,
gaearon,
ianschmitz,
iansu,
mrmckeb,
petetnt and
Timer
as code owners
|
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! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -35,9 +36,27 @@ function getBrowserEnv() { | |||
| } else if (value.toLowerCase() === 'none') { | |||
| action = Actions.NONE; | |||
| } else { | |||
| // Try detecting browser arguments | |||
| // Because Chrome for OSX is "google chrome", splitting simply by whitespace is not possible | |||
| if (value.startsWith(OSX_CHROME)) { | |||
There was a problem hiding this comment.
I believe other software may also contain of 2+ words with spaces. As I suggested in #7226, it would be simplier to add another environment variable for the browser arguments
There was a problem hiding this comment.
I really wanted this to be a non-invasive solution. I guess not...
There was a problem hiding this comment.
Isn't there another way, like hardcoding the possible browsers (special cases only)? It's more natural for someone to try passing arguments as one line, instead of using secondary environment variable.
There was a problem hiding this comment.
You also break the Single Responsibility Principle by mixing browser and it's arguments
There was a problem hiding this comment.
Another thing is that current implementation might be a breaking change. When you deal with open source, you might want to make sure people are not affected by the things they don't need at all.
|
@arvigeus, have you considered "BROWSER_ARGS" as an option? That would simplify this a lot I think :) |
|
@mrmckeb It would simplify things code-wise for sure. I wanted to try making it simpler in user's point of view, while also avoiding introducing new environment variables. It seems that it is not possible. Will redo it with additional var then. BROWSER_OPTIONS or BROWSER_ARGS? |
|
I think |
| | Variable | Development | Production | Usage | | ||
| | :---------------------- | :---------: | :--------: | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | BROWSER | ✅ Used | 🚫 Ignored | By default, Create React App will open the default system browser, favoring Chrome on macOS. Specify a [browser](https://github.com/sindresorhus/opn#app) to override this behavior, or set it to `none` to disable it completely. If you need to customize the way the browser is launched, you can specify a node script instead. Any arguments passed to `npm start` will also be passed to this script, and the url where your app is served will be the last argument. Your script's file name must have the `.js` extension. | | ||
| | BROWSER_ARGS | ✅ Used | 🚫 Ignored | Works in conjunction with BROWSER environment variable. When BROWSER is specified, any values set in BROWSER_ARGS will be passed to that browser instance. Example: `BROWSER_ARGS=--remote-debugging-port=9222`. Passing arguments directly to BROWSER won't work! | |
There was a problem hiding this comment.
I'd change the text here to:
When the
BROWSERenvironment variable is specified, any arguments that you set to this environment variable will be passed to the browser instance. Multiple arguments are supported as a space separated list. By default, no arguments are passed through to browsers.
I don't think we need an example, as this is a more advanced feature and I expect that anyone looking for it would be comfortable with it.
| | IMAGE_INLINE_SIZE_LIMIT | 🚫 Ignored | ✅ Used | By default, images smaller than 10,000 bytes are encoded as a data URI in base64 and inlined in the CSS or JS build artifact. Set this to control the size limit in bytes. Setting it to 0 will disable the inlining of images. | | ||
| | Variable | Development | Production | Usage | | ||
| | :---------------------- | :---------: | :--------: | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | BROWSER | ✅ Used | 🚫 Ignored | By default, Create React App will open the default system browser, favoring Chrome on macOS. Specify a [browser](https://github.com/sindresorhus/opn#app) to override this behavior, or set it to `none` to disable it completely. If you need to customize the way the browser is launched, you can specify a node script instead. Any arguments passed to `npm start` will also be passed to this script, and the url where your app is served will be the last argument. Your script's file name must have the `.js` extension. | |
There was a problem hiding this comment.
Would you mind updating the GitHub URL here?
https://github.com/sindresorhus/opn#app -> https://github.com/sindresorhus/open#app
|
The CI tests seem to be unrelated to this, so I'll merge. Great work @arvigeus! |
Closes #7226. Passing arguments to BROWSER environment variable like
google-chrome --remote-debugging-port=9222was breaking the build. The problem was that open requires arguments to be passed as array instead of plain string. This change solves that.