search

timarney / react-app-rewired

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

Resolve and load dependent module paths in react-scripts #530

Closed Yama-Tomo closed 2 years ago

Yama-Tomo commented 3 years ago

issue #457, #388

Depending on the project, the installation location of the package on which react-scripts depends may change. This fix resolves and loads the dependency paths of react-scripts.

liby commented 3 years ago

Will this PR solve the issue mentioned in this reply?

Yama-Tomo commented 3 years ago

Probably it will work. You can try it on my forked branch.

timarney commented 2 years ago

If @dawnmist or others can confirm this is needed --- happy to merge.

I haven't used Create React App or react-app-rewired for a number of years now so relying on others to let me know if this breaks or maintains things.

dawnmist commented 2 years ago

Does anyone have an example mini-repo that triggers the issue that I can use for testing the fix against? I can create a "normal" example, but not having used yarn workspaces it'll take me longer to try to put something together to test that the bug is actually fixed.

dawnmist commented 2 years ago

I've tested it with the repository at https://github.com/NiGhTTraX/react-app-rewired-bug-457 and can confirm that it works for the cross-spawn issue.

I think it may also work for the babel-jest hoisting issue mentioned in #589 - but the example repo above works in jest with the current master version of babelTransform (i.e. without the dependRequire/dependRequireResolve changes in babelTransform), so I can't be sure on that yet. The conflict that git is reporting is for line 3 in babelTransform.js, was altered to do the esModule vs default export testing in master, so it just needs the patch to be updated to apply against the current master as per the example I provided above.

Yama-Tomo commented 2 years ago

@dawnmist After fixing the conflicts, I applied my code to https://github.com/NiGhTTraX/react-app-rewired-bug-457 and confirmed that it works.

dawnmist commented 2 years ago

@Yama-Tomo Thank you for the rapid update!

@timarney - I can confirm that the changes work for both a new project with react-scripts 5, and on an older one with react-scripts 3, in addition to an example repo that was having issues finding the react-dev-utils/cross-spawn, so I'm happy for the changes to be merged.

dawnmist commented 2 years ago

Confirmed that the changes in this pull request also fix finding babel-jest when it is installed to node_modules/react-scripts/node_modules directory (due to hoisting or other packages having a conflicting version for babel-jest). See example repo https://github.com/Semigradsky/react-app-rewired-test.git for babel-jest testing.

timarney commented 2 years ago

Thanks All Published 2.1.10