timarney / react-app-rewired

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

fix: use default export on babel-jest 27 #589

Closed mrnerdhair closed 2 years ago

mrnerdhair commented 2 years ago

Should fix #585.

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

@reidrankin - Does this also work for people who have not yet updated to react-scripts to use babel-jest 26?

If not, it'd need to be released as a new major version for react-scripts so we don't break existing projects.

I won't have time to create example projects to test for a few days, but I didn't want to leave this without response until I did have a chance to get to it. I can see other people reporting issues with the babel-jest import - e.g. https://github.com/timarney/react-app-rewired/issues/587#issuecomment-998835530 - so I just want to be sure whether this was safe to roll out to the 2.x branch or if we'd need a 3.x branch of react-app-rewired for it.

mrnerdhair commented 2 years ago

This should be backwards-compatible; I'll test against 26 later today and confirm.

mrnerdhair commented 2 years ago

Just tested with babel-jest 26, can confirm that this patch is backwards-compatible!

dawnmist commented 2 years ago

Awesome. I'm happy for it to go through then - being a solution for both styles of import is really useful, thank you. :)

timarney commented 2 years ago

Merged and published
https://www.npmjs.com/package/react-app-rewired

Semigradsky commented 2 years ago

@timarney shouldn't babel-jest be added as dependency or peerDependency?

I currently have an error:

yarn run v1.22.5
$ CI=true react-app-rewired test
Error: Cannot find module 'babel-jest'
Require stack:
- .../frontend/node_modules/react-app-rewired/scripts/utils/babelTransform.js
- .../frontend/node_modules/jest-util/build/requireOrImportModule.js
- .../frontend/node_modules/jest-util/build/index.js
- .../frontend/node_modules/jest-config/build/getCacheDirectory.js
- .../frontend/node_modules/jest-config/build/Defaults.js
- .../frontend/node_modules/jest-config/build/normalize.js
- .../frontend/node_modules/jest-config/build/index.js
- .../frontend/node_modules/jest-cli/build/init/index.js
- .../frontend/node_modules/jest-cli/build/cli/index.js
- .../frontend/node_modules/jest-cli/build/index.js
- .../frontend/node_modules/jest/build/jest.js
- .../frontend/node_modules/react-scripts/scripts/test.js
- .../frontend/node_modules/react-app-rewired/scripts/test.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (.../frontend/node_modules/react-app-rewired/scripts/utils/babelTransform.js:1:21)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)

I think it is related to this.

P.S. I fixed it by temporarily adding babel-jest to my devDependencies.

dawnmist commented 2 years ago

@Semigradsky react-scripts has babel-jest as a dependency, so it ought to have been included according to the version that react-scripts requires. The issue is that it must be the correct version to match whichever jest version that react-scripts includes, so adding it hard-coded to specific versions in react-app-rewired means that depending on which version of react-scripts the user is currently using we'd need to specify different versions in react-app-rewired. Leaving it to react-scripts to specify means that it would always be matching the version of jest that react-scripts specifies, so the two are not out-of-sync (it often causes API interface incompatibilities if the version of jest and babel-jest don't match).

Your output above looks like you may be using yarn workspaces - is that correct? If so, I suspect you've likely run into a hoisting issue within your project (i.e. babel-jest either not being hoisted, or the hoisted version not being found correctly).

Semigradsky commented 2 years ago

...so adding it hard-coded to specific versions in react-app-rewired means that depending on which version of react-scripts the user is currently using we'd need to specify different versions in react-app-rewired

So babel-jest should be added as range in peerDependency?

...looks like you may be using yarn workspaces

I haven't use yarn workspaces, but it looks like hoisting issue.

dawnmist commented 2 years ago

@Semigradsky

...so adding it hard-coded to specific versions in react-app-rewired means that depending on which version of react-scripts the user is currently using we'd need to specify different versions in react-app-rewired

So babel-jest should be added as range in peerDependency?

When done as a range it'll need maintenance every time CRA has a new major release to update the range to cover the version used in the new CRA release. That's part of what I was hoping to avoid, since I don't typically update my own projects immediately and Tim doesn't use react-app-rewired at all anymore, so between the two of us we wouldn't notice the need to update the version range until people lodged bugs against react-app-rewired. I was hoping to keep react-app-rewired as low-maintenance as possible so that it mostly "just works" when react-scripts updates without needing adjustment every time.

There's also been many issues in the past caused by people installing different versions of babel-jest vs jest, and I can see people raising bugs again in future due to misunderstanding the purpose of the range specification - if react-app-rewired claims it runs with "any of major versions C,D,E,F of babel-jest" (replacing numbers with characters just as simplification), some people will try to mix-and-match versions when the issue really is that since they're using jest version "E" they are required to use babel-jest version "E" as well so installing babel-jest version "F" is causing their incompatibility. I've answered so many of those types of issues over time that I'm hesitant to add something that will cause confusion for some developers when having react-scripts itself installed usually means that the person will have the correct version of babel-jest already installed as part of react-script's dependencies.

I think that the pull request #530 may fix the issue of not finding the hoisted version of babel-jest without needing to apply a peer dependency range to react-app-rewired, so that it'll continue to work with future versions of react-scripts. Are you able to test if the patch works for babel-jest in your project without needing the peer dependency? There is one minor alteration to the pull request required so that babel-jest versions 26 + 27 both work - basically updating line 3 of the babelTransform.js file in the pull request as per the comment: https://github.com/timarney/react-app-rewired/pull/530#discussion_r775142455 . Alternatively, are you able to create/share an example repo that I can test against that exhibits the babel-jest hoisting/resolution issue that you are experiencing so that I can verify if the changes in #530 do resolve the issue you are having with babel-jest as well?

Semigradsky commented 2 years ago

@dawnmist oh, sorry to bother you. I found that I forget to remove resolution "babel-preset-react-app": "10.0.0" (10.0.1 has changes that doesn't work with previous version of CRA). There is no hoisting issue after removing resolution 👍

In any case, I created an example repo: https://github.com/Semigradsky/react-app-rewired-test I tried changes from #530 and they help for this case.

dawnmist commented 2 years ago

@Semigradsky thank you for the example repo, and I am glad that you've found the cause locally for your hoisting issue. The repo lets me validate that #530 does indeed fix issues with babel-jest with hoisting, so it is really useful. Thank you. :)