timarney / react-app-rewired

Override create-react-app webpack configs without ejecting
MIT License
9.77k stars 425 forks source link

fix: Premature removal of the --config-overrides CLI argument #595

Closed bencergazda closed 2 years ago

bencergazda commented 2 years ago

Cannot start test script if the config-overrides path is set via command-line argument, because it gets removed before the final paths config is done.

Steps to reproduce:

The test should not start, even with a correct path, because the config-overrides argument gets stripped out from the process.argv array during initialization, and tries to load the config-overrides.js from its default location (the project root).

The issue doesn't occur

This hotfix uses the same pattern as we have for removing the --scripts-version argument.

yifanwww commented 2 years ago

I also faced this problem but in build script, I guess start script will also fail.

Here I just make a description about why this problem happens.

After the release of v2.1.10, paths.js will be required two times, and the --config-overrides path will get deleted when we require this file (see line 22).

Before spawning a new process to execute build, start or test, bin/index.js will require paths.js (via dependRequire.js). So when it start to spawn a new process, the variable nodeArgs passed to the new process no longer contains the --config-overrides argument.

And that's why this MR can fix it, the line 22 is deleted.

bencergazda commented 2 years ago

@yifanwww This is definitely true, thanks for the clarification.

lidaof commented 2 years ago

I am also having this issue I removed line 22 seems make the --config-overrides argument to work again... this seems not merged...any other fix? otherwise need to modify the file on every machine i guess?

yifanwww commented 2 years ago

@lidaof Don't know when this pr can be merged... you can use version 2.1.9 if you don't need the new features or bug fixes in v2.1.10 and v2.1.11

lidaof commented 2 years ago

Thank you @yifanwww for reply. Looks like 2.1.9 still has this line in the file?

yifanwww commented 2 years ago

@lidaof Yes it has. But the file dependRequire.js doesn't exist, so paths.js is only required once and only after spawning the new process. So --config-overrides still exists when we need it.

lidaof commented 2 years ago

That's great to know, thank you so much! @yifanwww

timarney commented 2 years ago

I'll likely merge #593 on the weekend and this one as well

After that I'll publish the next version

https://github.com/timarney/react-app-rewired/pull/593#issuecomment-1022078672

cc: @dawnmist

timarney commented 2 years ago

I'll get this merged + pushed up tonight.

timarney commented 2 years ago

Conflicts need to be fixed first

bencergazda commented 2 years ago

@timarney I am going to submit an updated version in a few hours.

bencergazda commented 2 years ago

@timarney Now it is ready to merge