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

fix: browser selection via env variable #11878

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FloydJohn
Copy link

@FloydJohn FloydJohn commented Jan 3, 2022

Hi everyone,

this PR fixes the opening of a browser of choice, selected through the BROWSER environment variable, when starting a generated project.

After migrating to CRA v5.x the feature stopped working (see #11873, #11844, #11942, #11917, #12275).

Browsing the documentation for the open@8.4.0 package (available here) I noticed that the syntax for opening an URL using a specified browser was updated.

This PR includes two small changes:

  • The updated syntax for opening a browser is now used
  • Since the open package natively supports passing an array of arguments to the application, we no longer need to concatenate the arguments before the call

To test this feature I followed this test plan:

  • Create a new .env file in the template project, containing for example the value BROWSER=google-chrome-stable. The browser name could change based on OS or linux distro.
  • Start the template project with npm run start. A new tab should open in the specified browser.
  • Stop the process and remove the newly created .env file.
  • Start again the project with npm run start: it should open on the default browser.

@facebook-github-bot
Copy link

Hi @FloydJohn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign 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 to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@Karthick-FK
Copy link

Karthick-FK commented Jan 24, 2022

@iansu @mrmckeb Can anyone of you look at this PR?

@4nkit-5hukla
Copy link

This Process still not working.
in my .env.local I put BROWSER=firefox

@FloydJohn
Copy link
Author

HI @4nkit-5hukla, thanks for the feedback!

I tested this fix again, and I can confirm that BROWSER=firefox correctly launches the browser on my Linux and Windows installs.

I'll leave here the steps to test this fix:

  • As this is not merged, you'll have to clone my fork on the correct branch: git clone https://github.com/FloydJohn/create-react-app -b fix-browser-env
  • Create a .env file here: packages/cra-template/template/.env
  • Insert in this .env file the line BROWSER=firefox. Test that on your terminal you can start Firefox by issuing the firefox command
  • Launch from the repo root npm i && npm run start

Firefox should open as expected.

To make sure that you created the .env file in the correct path, you can always add a variable like REACT_APP_TEST=test and display the loaded env variables by adding a line like {JSON.stringify(process.env)} on the template.

Hope that this helped, let me know if it works as expected!

@renaatdemuynck
Copy link

After having this problem myself, I did some debugging and came to the same conclusion. I was going to create a pull request myself, but @FloydJohn beat me to it. I can confirm that this fix solves the problem. (Another fix would be to revert to open@7.4.2.)

@martiniil
Copy link

I can confirm that this fixes #11917.

@@ -120,8 +120,6 @@ function startBrowserProcess(browser, url, args) {
try {
var options = {
app: { name: browser, arguments: args },
wait: false,
Copy link

Choose a reason for hiding this comment

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

Did you mean to delete the wait property?

Copy link
Author

Choose a reason for hiding this comment

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

Yes! I noticed the default is set to false, as described here

Copy link

Choose a reason for hiding this comment

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

cool

dlech added a commit to pybricks/pybricks-code that referenced this pull request May 16, 2022
@renaatdemuynck
Copy link

renaatdemuynck commented May 16, 2022

TIP: For now, this works as a temporary fix in package.json:

  "resolutions": {
    "open": "7.4.2"
  },

@antingle
Copy link

antingle commented Sep 7, 2022

Is there an update on merging this PR?

@tmanson
Copy link

tmanson commented Sep 26, 2022

When this PR will be merge ?

@chrisschuller
Copy link

@iansu @mrmckeb Please merge when you have time :)

@AFCMS
Copy link

AFCMS commented Oct 18, 2022

Could this be merged? Its a very annoying issue.

@rs7
Copy link

rs7 commented Nov 9, 2022

Up

@wahabmirjan
Copy link

Hello. Is there an update on when would this be ready? Much appreciated. Thanks.
@iansu @mrmckeb

@OmKakatkar
Copy link

Any updates on merging this PR? @iansu @mrmckeb

@elringus
Copy link

TIP: For now, this works as a temporary fix in package.json:

  "resolutions": {
    "open": "7.4.2"
  },

This workaround didn't work with npm for me. Instead I've used the following (requires npm-cli 8.3 or later):

    "overrides": {
        "open": "7.4.2"
    }

I had to also remove node_modules and package-lock.json before running npm install for the override to work.

@DamienCassou
Copy link

It would be nice to merge this PR.

@sKopheK
Copy link

sKopheK commented Aug 6, 2023

hi, why is this still not merged?

@kovryzhenko
Copy link

merge asap wtf ffs. what r u waiting for ? :D

@sKopheK
Copy link

sKopheK commented Aug 20, 2023

look at when the last version was released: https://www.npmjs.com/package/create-react-app
(a year ago)
seems like the project is not really alive, better switch to some modern boilerplate

@kovryzhenko
Copy link

look at when the last version was released: https://www.npmjs.com/package/create-react-app (a year ago) seems like the project is not really alive, better switch to some modern boilerplate

whats ur choice bro?

@sKopheK
Copy link

sKopheK commented Aug 29, 2023

look at when the last version was released: https://www.npmjs.com/package/create-react-app (a year ago) seems like the project is not really alive, better switch to some modern boilerplate

whats ur choice bro?

for one existing project based on CRA i've switched to vite, so far better. but im not that experienced with these tools so can only recommend it based on how the project is active

@BraianS
Copy link

BraianS commented Mar 17, 2024

Goodnight. Any updates on this pull request?

@kovryzhenko
Copy link

????????? cra dead or what ?

@BraianS
Copy link

BraianS commented May 30, 2024

????????? Are you dead or what?

Good night. Some people say "yes" and use Vite as an alternative tool....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.