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

jest-runtime: move babel-core to peer dependencies #4557

Merged
merged 1 commit into from Sep 28, 2017

Conversation

@vlad-zhukov
Copy link

@vlad-zhukov vlad-zhukov commented Sep 28, 2017

Summary

It's still not possible to use Jest with Babel 7 due to some kind of a conflict between Babel versions because jest-runtime requires babel-core@^6. Moving babel-core to peer dependencies such as in babel-jest resolves the problem.

Test plan

Existing tests should pass.

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Sep 28, 2017

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up 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 the corporate CLA signed.

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

@vlad-zhukov vlad-zhukov changed the title jest-runtime: make babel-core a peer dependency jest-runtime: move babel-core to peer dependencies Sep 28, 2017
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Sep 28, 2017

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 28, 2017

Codecov Report

Merging #4557 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4557   +/-   ##
=======================================
  Coverage   56.16%   56.16%           
=======================================
  Files         185      185           
  Lines        6285     6285           
  Branches        3        3           
=======================================
  Hits         3530     3530           
  Misses       2754     2754           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80bd4cd...8aac13a. Read the comment docs.

@cpojer cpojer merged commit 1fcffab into facebook:master Sep 28, 2017
7 checks passed
7 checks passed
ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-4 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tabrindle added a commit to tabrindle/jest that referenced this pull request Oct 2, 2017
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 24, 2017

This change means that you get a peer dep warning if you do not depend on babel-core somewhere in your tree.

Then you have to install it manually, or tests WILL fail to execute.

 FAIL  ./test.js
  ● Test suite failed to run

    Cannot find module 'babel-core'

      at Function.Module._resolveFilename (module.js:527:15)
      at Function.Module._load (module.js:476:23)
      at Module.require (module.js:568:17)
      at require (internal/module.js:11:18)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.684s
Ran all test suites.

Not sure how to handle it. It's not really nice that people not using babel at all will have to install babel-core manually just to be able to use Jest.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 24, 2017

@hzoo Your input would be greatly appreciated here.

Issue:

  • Jest depends on babel-core (as it uses babel for code coverage and hoisting in order to do mocks)
  • Jest also picks up a project's babel config so things just work™️ without a require hook or something like that
  • we want to allow users to use babel@7 if they want
  • we don't want to force users to upgrade from 6 to 7 in order to use jest
  • we don't want to force users who don't already depend on babel (lots of node only projects for instance, and not every web project uses babel either) to install babel-core

Is there some way you know of to achieve the following:

  • If the user has installed either babel-core@6 or babel-core@7, use that
  • If the user has not installed babel-core at all, install babel-core@latest somehow
@frenzzy
Copy link

@frenzzy frenzzy commented Nov 2, 2017

Babel packages were moved to npm scope since v7 beta 5, could we add support for future versions of babel by using latest babel if available? Maybe something like this:

let transform;
let babelTransform;
try {
  ({ transform, babelTransform } = require('@babel/core'));
} catch (error) {
  ({ transform, babelTransform } = require('babel-core'));
}
// ...
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 2, 2017

Auch, that actually makes it even worse for us.

@cpojer maybe we add babel-core as a dep in jest, and do module.parent.require? Then we get the user's dep if they define one, but our own if they don't (as it's hoisted to the top-level)

@loganfsmyth
Copy link

@loganfsmyth loganfsmyth commented Nov 2, 2017

Since jest-runtime on its own doesn't read the user's config (babelrc: false), it doesn't seem like it should need to transition to Babel 7 at any particular time to me. This issue doesn't seem to clearly spell out what was broken when used with babel 7 at the project root. 6 should have just been installed in a nested node_modules right?

we don't want to force users who don't already depend on babel (lots of node only projects for instance, and not every web project uses babel either) to install babel-core

Does that apply to jest-runtime only, or babel-jest too?

For babel-jest things do get more complicated because of the @babel scope. The option I've thrown out here, is that we make kind of "bridge" packages to allow for gradual transition for packages that have the peerDep on babel-core instead of @babel/core. Let me know if this makes sense.

For babel-jest we expand its peerDeps with 7.0.0-bridge

"babel-core": "^6.0.0 || ^7.0.0-alpha || ^7.0.0-beta || ^7.0.0 || 7.0.0-bridge"

then this bridge package could have its own peerDependency on @babel/core and essentially do module.exports = require("@babel/core"); so people who want to use Babel 7.x on existing Jest versions would do

npm install babel-jest babel-core@7.0.0-bridge @babel/core

to get all three packages. Otherwise the existing npm install babel-jest babel-core would still work for 6.x.

Then we could also do the opposite so once packages change the peerDep to @babel/core people wanting to use 6.x could do

npm i babel-jest @babel/core@6.0.0-bridge babel-core`

thoughts?

@vlad-zhukov
Copy link
Author

@vlad-zhukov vlad-zhukov commented Nov 3, 2017

@loganfsmyth The problem was that tests failed due to syntax errors (such as ones that point to import declarations) which means that babel in jest didn't do it's job.

6 should have just been installed in a nested node_modules right?

It kind of did, but yarn weirdly hoisted a few deeply nested packages (haven't tested in npm).

Also what about .babelrc and .babelrc.js?

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 3, 2017

Does that apply to jest-runtime only, or babel-jest too?

Both. Jest uses babel for a couple of things:

(I think that's it)

I like the idea of a bridge module. What I really want is to use the user's installed babel-core if it exists. If it does not, I want to provide my own. I guess a bridging module combined with module.parent.require(which is admittedly pretty hacky) would solve it?

@loganfsmyth
Copy link

@loganfsmyth loganfsmyth commented Nov 3, 2017

Let me break down my thoughts a bit more. The two places:

  • In jest-runtime
    For this case, you're passing babelrc: false, with a hardcoded list of a single plugin. Given that, the specific version of Babel used in this case is 100% up to you because you control both the inputs and outputs of Babel's configuration. To me that means that this PR is incorrect, because you control the inputs and outputs, you should be safe to hard-code a specific Babel version. There is nothing to gain by letting the user control the version of Babel in use here.

  • In babel-jest
    This is the harder case, because you don't pass babelrc: false so it will use the user's .babelrc config. In my opinion the peerDependency of "babel-core": "6.x | 7.0.0-beta | 7.0.0-bridge" seems reasonable here, because babel-jest is already a package that users have to opt into separately from Jest itself, so requiring them to choose the Babel version for this usecase makes sense to me. If they choose v6, then npm will be able to flatten the v6 version from jest-runtime into the root and they will share the same core, and if the user installs v7 as a peerDependency of babel-jest, then jest-runtime will end up with v6 installed as its own nested dependency.

I think given that, you can just rely on the user to install the correct babel-core that correlates with their .babelrc (which I'd assume will already be installed for use in babel-loader or babel-cli anyway). I don't know how I feel about automatically using the user's babel-core if it exists, but you could certainly add some logic like that to warn the user somehow if they've installed @babel/core in their root, but forgotten to install the bridge package meaning babel-jest would still try to use v6.

@borela
Copy link

@borela borela commented Nov 12, 2017

Was the bridge package created?

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 13, 2017

@cpojer thoughts here?

@cpojer
Copy link
Contributor

@cpojer cpojer commented Nov 13, 2017

I think it would be great if we could stop using babel-jest in jest-runtime but instead use babel for code coverage more directly. Could we make that happen?

@borela
Copy link

@borela borela commented Nov 13, 2017

@cpojer We could make a bridge package babel-jest-scoped or babel-jest-v7 just to make it work with the scoped babel packages for now and then later try to eliminate babel jest in a major release.

@cpojer
Copy link
Contributor

@cpojer cpojer commented Nov 13, 2017

I'm not sure what the best solution is but it makes sense to me that:

  • jest-runtime uses one version of babel to make code coverage work
  • babel-jest works with either babel 6 or babel 7

We can make the change to both right now because the next Jest release will be a major.

@borela
Copy link

@borela borela commented Nov 13, 2017

I personaly liked @loganfsmyth's idea of having a bridge package as a way to select which core to load. For now I edited my local jest package to load @babel/core instead of babel-core and it worked flawlessly.

@loganfsmyth
Copy link

@loganfsmyth loganfsmyth commented Nov 13, 2017

I've just now published babel-core@7.0.0-bridge.0 that has its own peerDep on @babel/core@^7.0.0-0 so you should now be able to have babel-jest do

"peerDependencies": {
  "babel-core": "6.x | ^7.0.0-0"
}

as a generic block to support all of 6.x and 7.x, including the 7.x alphas, betas, and the new bridge package. You'll just need some docs for users on how to install the bridge package, if they want to use @babel/core as

npm install babel-jest babel-core@^7.0.0-0 @babel/core
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 20, 2017

PR, for people following this: #4918.

Should we revert this PR?

EDIT: Revert: #4923

SimenB added a commit to SimenB/jest that referenced this pull request Nov 20, 2017
)"

This reverts commit 1fcffab.
cpojer added a commit that referenced this pull request Nov 20, 2017
…4923)

* Revert "jest-runtime: move babel-core to peer dependenies (#4557)"

This reverts commit 1fcffab.

* Update changelog
@alfonsoperez
Copy link
Contributor

@alfonsoperez alfonsoperez commented Nov 23, 2017

Awesome guys!, this is exciting!.

For the time being, I have it working with jest@test and babel-7-jest

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 23, 2017

Do you need the second package?

It would be great to have an example in the docs with how to use babel 7 and jest together

@danez
Copy link
Contributor

@danez danez commented Nov 23, 2017

I added it in my PR see https://facebook.github.io/jest/docs/en/getting-started.html#using-babel
This already works with latest stable version, but you get peerDependency warnings

@loganfsmyth
Copy link

@loganfsmyth loganfsmyth commented Nov 23, 2017

I believe this has been published in babel-jest@21.3.0-beta.9 right? So babel-7-jest shouldn't be needed.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 23, 2017

babel-runtime and babel-jest betas were published yesterday, and contain the fixes, yes. I haven't tested it, though

@alfonsoperez
Copy link
Contributor

@alfonsoperez alfonsoperez commented Nov 24, 2017

hmm I might be dumb then, because:

  "devDependencies": {
    "@babel/cli": "^7.0.0-beta.32",
    "@babel/core": "^7.0.0-beta.32",
    "@babel/node": "^7.0.0-beta.32",
    "@babel/plugin-syntax-dynamic-import": "^7.0.0-beta.32",
    "@babel/plugin-syntax-object-rest-spread": "^7.0.0-beta.32",
    "@babel/plugin-transform-flow-strip-types": "^7.0.0-beta.32",
    "@babel/preset-env": "^7.0.0-beta.32",
    "@babel/register": "^7.0.0-beta.32",
    "babel-7-jest": "^21.3.1",
    "babel-eslint": "^8.0.2",
    "eslint": "^4.1.0",
    "eslint-config-prettier": "^2.8.0",
    "eslint-plugin-flowtype": "^2.34.0",
    "eslint-plugin-jest": "^21.3.2",
    "flow-bin": "^0.59.0",
    "flow-remove-types": "^1.2.1",
    "flow-typed": "^2.2.3",
    "jest": "^21.3.0-beta.9",
    "prettier": "^1.4.4"
  }
➜  test git:(master) ✗ yarn test
yarn test v1.0.2
$ jest --runInBand src/
 PASS  src/__tests__/utils.test.js
 test git:(master) ✗ yarn remove babel-7-jest; yarn add babel-jest@21.3.0-beta.9 --dev
➜  test git:(master) ✗ yarn test
yarn test v1.0.2
$ jest --runInBand src/
 FAIL  src/__tests__/utils.test.js
  ● Test suite failed to run

    TypeError: Cannot read property 'loose' of undefined (While processing preset: "/Users/alfonsoperez/repos/test/node_modules/@babel/preset-env/lib/index.js")

@danez
Copy link
Contributor

@danez danez commented Nov 24, 2017

you are missing babel-core@^7.0.0-0

@alfonsoperez
Copy link
Contributor

@alfonsoperez alfonsoperez commented Nov 24, 2017

@danez, I wasn't sure about that one :D, and I installed instead @babel/core since

➜  test git:(master) ✗ npm install --save-dev babel-jest babel-core@^7.0.0-0 @babel/core

zsh: no matches found: babel-core@^7.0.0-0

??

@danez
Copy link
Contributor

@danez danez commented Nov 24, 2017

That seems to come from your zsh, that tries to do something with the command, maybe need to escape the ^

@alfonsoperez
Copy link
Contributor

@alfonsoperez alfonsoperez commented Nov 24, 2017

Ohh you are right!, I missed that, thanks!, and sorry for the noise then!

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Nov 24, 2017

Should we put the command in quotes in the readme so it's more copy-pasteable?

PR welcome! 🙂

@clemmy clemmy mentioned this pull request Nov 30, 2017
7 of 7 tasks complete
afc163 added a commit to ant-design/ant-design-pro that referenced this pull request Jan 9, 2018
hiroppy added a commit to hiroppy/babel-upgrade that referenced this pull request Feb 21, 2018
@skidding
Copy link

@skidding skidding commented Apr 25, 2018

It's great that we have a workaround.

But is there any plan to make babel-jest work with @babel/core directly and not have to install babel-core@bridge as well? If so I'd like to follow that thread. Thanks.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Apr 25, 2018

Not for now as that would stop babel 6 from working. After babel 7 is promoted to stable that might change, but for now we'll keep the current behaviour

@ryami333
Copy link

@ryami333 ryami333 commented Sep 3, 2018

Gidday @SimenB - now that Babel 7 is stable has this progressed at all? babel-jest is now the only dependency in my project with babel-* peer-dependencies since I've upgraded to Babel 7, and it would be nice to be able to remove that.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Sep 7, 2018

See #6949

@chaimleib
Copy link

@chaimleib chaimleib commented Jan 25, 2019

What if we could use @babel/jest? This would make the bridge unnecessary, and jest-runtime could import babel via @babel/jest.

In other words, how about if we combine the bridge with babel-jest to produce @babel/jest, following the new babel 7 naming convention?

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Jan 25, 2019

Install Jest 24, released today, and you don't need the bridge module anymore - just use @babel/core

@sumantka
Copy link

@sumantka sumantka commented Feb 18, 2019

@SimenB could you expand on 'just use @babel/core' What does that mean. I've installed @babel/core and tried almost everything and at the point of giving up.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Feb 18, 2019

Please see the docs: https://jestjs.io/docs/en/getting-started#using-babel

Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

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

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.