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: use default export on babel-jest 27 #589

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

mrnerdhair
Copy link
Contributor

Should fix #585.

@timarney
Copy link
Owner

timarney commented Dec 21, 2021

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
Copy link
Collaborator

@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. #587 (comment) - 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
Copy link
Contributor Author

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

@mrnerdhair
Copy link
Contributor Author

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

@dawnmist
Copy link
Collaborator

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

@timarney timarney merged commit cc0e3d9 into timarney:master Dec 22, 2021
@timarney
Copy link
Owner

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

@mrnerdhair mrnerdhair deleted the patch-1 branch December 22, 2021 15:17
@Semigradsky
Copy link

Semigradsky commented Dec 24, 2021

@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
Copy link
Collaborator

@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
Copy link

...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
Copy link
Collaborator

dawnmist commented Dec 25, 2021

@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: #530 (comment) . 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
Copy link

Semigradsky commented Dec 25, 2021

@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
Copy link
Collaborator

@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. :)

jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jan 24, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jan 24, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jan 28, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Feb 7, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Mar 10, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Apr 14, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request May 16, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 3, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 7, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 8, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 8, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 8, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 13, 2022
jmthibault79 added a commit to all-of-us/workbench that referenced this pull request Jun 13, 2022
* bump react-scripts to 5.0.0
* bump react-app-rewired to include timarney/react-app-rewired#589
* polyfill querystring -> querystring-es3
* install url
* add eslint-plugin-react-hooks
* can remove a few forced resolutions
* un-alias ansi-html-community to fix compile issue
* await fix
* test fix
* filter out CssMinimizerPlugin
* rm mutationobserver-shim to silence warning
* bump webpack
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

babel-jest phantom dependency
4 participants