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

chore(breaking): remove regenerator-runtime injection #7595

Merged
merged 11 commits into from Jan 20, 2019

Conversation

Projects
None yet
5 participants
@thymikee
Copy link
Collaborator

thymikee commented Jan 9, 2019

Summary

An attempt to get rid of regenerator-runtime addition in setupFiles. It's currently not working, but wanted @SimenB to see what can be wrong.

Test plan

Fix automock test failures

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

Why do we need the regenerator runtime at all? If it's our own code, shouldn't the babel transform add whatever it needs to transpile async? If it's for users, why can't they setup transpilation of async-await themselves?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

As for the test failures, the changed stack trace looks awesome! Not sure about the automock ones, I'll take a look

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Jan 10, 2019

I think it's there for historic reasons, to make Jest "just work" with Babel's generators but I'd need @cpojer to back this up.

With Babel 6 it required globally available regenerator-runtime, but since Babel 7 it looks like regenerator-runtime is hidden inside @babel/runtime and accessible through the plugin and users should use that? Why it's not included in the @babal/preset-env though?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

See #7599

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

to make Jest "just work" with Babel's generators

Now that node 8+ supports async-await out of the box, I'm not sure this makes sense. Seems like people should configure Babel properly instead if they want node 6?

@thymikee

This comment has been minimized.

Copy link
Collaborator Author

thymikee commented Jan 10, 2019

Yup, agreed

@thymikee thymikee force-pushed the thymikee:feat/use-transform-runtime branch from 25ccdfd to f3ca428 Jan 10, 2019

Show resolved Hide resolved package.json Outdated

@thymikee thymikee force-pushed the thymikee:feat/use-transform-runtime branch from 9fd72dd to 6abd74f Jan 10, 2019

Show resolved Hide resolved examples/async/package.json Outdated
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

Should update the title of this PR, as well as update the changelog (marking it as breaking, I guess)

@SimenB SimenB added this to the Jest 24 milestone Jan 10, 2019

@thymikee thymikee force-pushed the thymikee:feat/use-transform-runtime branch from 6abd74f to 3db5e9f Jan 10, 2019

@thymikee thymikee changed the title [WIP] feat: use @babel/plugin-transform-runtime instead of regenerator-runtime feat(breaking): remove regenerator-runtime injection Jan 10, 2019

@thymikee thymikee changed the title feat(breaking): remove regenerator-runtime injection chore(breaking): remove regenerator-runtime injection Jan 10, 2019

@thymikee thymikee force-pushed the thymikee:feat/use-transform-runtime branch 2 times, most recently from 00d0931 to 1c72e35 Jan 10, 2019

Show resolved Hide resolved yarn.lock Outdated
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 10, 2019

Codecov Report

Merging #7595 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7595      +/-   ##
==========================================
- Coverage   68.24%   68.16%   -0.08%     
==========================================
  Files         251      251              
  Lines        9639     9634       -5     
  Branches        5        5              
==========================================
- Hits         6578     6567      -11     
- Misses       3059     3065       +6     
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-config/src/normalize.js 82.3% <100%> (-0.72%) ⬇️
packages/jest-runtime/src/index.js 75.51% <0%> (-1.48%) ⬇️

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 5fc391f...df4350f. Read the comment docs.

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

Woo, green 😀

@SimenB

SimenB approved these changes Jan 10, 2019

@SimenB SimenB requested review from cpojer , rubennorte and rickhanlonii Jan 10, 2019

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Jan 15, 2019

I'm ok with this change. @thymikee is indeed right, this was for making Babel and async/await work out of the box in the past. If this works fine with babel 7, I'm ok with it. Just make sure that if people compile async/await down to ES5 + regenerator runtime in their setup, this change won't break them (alternatively ask them to just drop the transform in the test environment, but it may require setup).

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 15, 2019

Users who just use @babel/plugin-transform-regenerator will get this error:
image

But it's not Jest's fault that they do not include it - they'd get the same error when using webpack or whatever and opening their JS in chrome.

They should use @babel/preset-env instead, IMO, and I think it's fine to break them. Just me? 😅

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 15, 2019

Adding @babel/plugin-transform-runtime works, which is mentioned in the docs: https://babeljs.io/docs/en/babel-plugin-transform-runtime#regenerator-aliasing

(just make sure to set sourceType to either script or unambiguous, otherwise the injected helper uses import)

module.exports = {
  plugins: [
    '@babel/plugin-transform-regenerator',
    '@babel/plugin-transform-runtime',
  ],
  sourceType: 'script',
};

An alternative to setting sourceType is to use @babel/plugin-transform-modules-commonjs


I'll push a test with that as an integration test

SimenB and others added some commits Jan 15, 2019

Merge remote-tracking branch 'upstream/master' into feat/use-transfor…
…m-runtime

* upstream/master:
  add missing truncate comment to recent blog posts (#7655)
  use raw serializer for e2e output snapshots (#7651)
  chore: use a Set for reserved words list in `jest-mock`
  Fix automock for numeric function names (#7653)
  Update docs re: `moduleFileExtensions` to add ordering note (left-to-right) (#7616)

@thymikee thymikee merged commit 64b3a9b into facebook:master Jan 20, 2019

12 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 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
ci/circleci: test-or-deploy-website 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
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190120.3 succeeded
Details

@thymikee thymikee deleted the thymikee:feat/use-transform-runtime branch Jan 20, 2019

@jsnajdr jsnajdr referenced this pull request Feb 15, 2019

Merged

chore(deps): update jest monorepo (major) #30434

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment