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

feat: pass ESM options to transformers #9597

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 19, 2020

Summary

When jest-runtime supports ESM, then babel can leave those statements alone. While not usable yet, this helps reduce the eventual diff for ESM.

Test plan

Added some unit tests to some funky merging code, otherwise this is not really testable until we land more ESM stuff. While it's all added as optional to avoid a breaking change, jest-runtime explicitly opts out if it, so no real way of triggering this.

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #9597 into master will increase coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9597      +/-   ##
==========================================
+ Coverage   65.09%   65.11%   +0.01%     
==========================================
  Files         287      287              
  Lines       12145    12151       +6     
  Branches     3009     3015       +6     
==========================================
+ Hits         7906     7912       +6     
  Misses       3604     3604              
  Partials      635      635
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 65.75% <ø> (ø) ⬆️
packages/jest-transform/src/ScriptTransformer.ts 70.17% <66.66%> (+0.53%) ⬆️
packages/babel-jest/src/index.ts 52.94% <80%> (+2.94%) ⬆️

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 c2a879d...a7fac13. Read the comment docs.

@@ -90,6 +90,8 @@ export default class ScriptTransformer {
fileData: string,
filename: Config.Path,
instrument: boolean,
supportsDynamicImport: boolean,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made them non-optional for all private methods and default false for the public ones

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should ideally use options objects, as 2-3 boolean flag inputs are quite confusing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally agree, but IIRC functions with more arguments are optimized better than the ones with object arguments, and this is kinda hot path

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially we could add separate functions? the es module path can be async (or rather, always is) as well, so that might tie nicely into #9504. Then we could ditch the supportsDynamicImport option since that works for both esm and cjs (and can be async) and just vary on the static version

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would reduce tests duplication with test.each, but other than that looks fine

packages/babel-jest/src/index.ts Outdated Show resolved Hide resolved
packages/babel-jest/src/loadBabelConfig.ts Outdated Show resolved Hide resolved
@@ -90,6 +90,8 @@ export default class ScriptTransformer {
fileData: string,
filename: Config.Path,
instrument: boolean,
supportsDynamicImport: boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally agree, but IIRC functions with more arguments are optimized better than the ones with object arguments, and this is kinda hot path

@SimenB SimenB force-pushed the pass-esm-options-to-transformers branch from 5f6eac4 to a7fac13 Compare February 20, 2020 15:19
@SimenB SimenB force-pushed the pass-esm-options-to-transformers branch from a7fac13 to 6aa31e9 Compare April 4, 2020 14:11
@SimenB SimenB force-pushed the pass-esm-options-to-transformers branch from 6aa31e9 to 17285a9 Compare April 5, 2020 09:06
@SimenB
Copy link
Member Author

SimenB commented Apr 5, 2020

Extracted the babel-jest stuff to #9766, makes this PR a bit cleaner, and I don't dislike the approach as much anymore

@@ -498,6 +498,8 @@ class Runtime {
return {
...options,
...this._coverageOptions,
supportsDynamicImport: false,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have been in #9766, meh

@@ -350,12 +380,16 @@ export default class ScriptTransformer {

private _transformAndBuildScript(
filename: Config.Path,
options: Options | null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to have been a bug, it was never passed null 🤷‍♂

@SimenB SimenB force-pushed the pass-esm-options-to-transformers branch from 17285a9 to 75fd400 Compare April 5, 2020 09:35
@SimenB SimenB force-pushed the pass-esm-options-to-transformers branch from 75fd400 to 2f163d9 Compare April 5, 2020 09:43
@SimenB SimenB merged commit edf8c0a into jestjs:master Apr 5, 2020
@SimenB SimenB deleted the pass-esm-options-to-transformers branch April 5, 2020 10:15
jeysal added a commit to mmkal/jest that referenced this pull request Apr 10, 2020
…pshots

* upstream/master: (225 commits)
  docs: add CLA link to contributing docs (jestjs#9789)
  chore: roll new version of docs
  v25.3.0
  chore: update changelog for release
  chore(jest-types): correct type testRegex for ProjectConfig (jestjs#9780)
  feat(circus): enable writing async test event handlers (jestjs#9397)
  feat: enable all babel syntax plugins (jestjs#9774)
  chore: add helper for getting Jest's config in e2e tests (jestjs#9770)
  feat: pass ESM options to transformers (jestjs#9597)
  chore: replace `any`s with `unknown`s (jestjs#9626)
  feat: pass ESM options to Babel (jestjs#9766)
  chore(website): add copy button the code blocks (jestjs#9750)
  chore: bump istanbul-reports for new uncovered lines design (jestjs#9758)
  chore: correct CHANGELOG.md (jestjs#9763)
  chore(jest-types): expose type `CacheKeyOptions` for `getCacheK… (jestjs#9762)
  docs: Fix simple typo, seperated -> separated (jestjs#9760)
  v25.2.7
  chore: update changelog for release
  fix: drop getters and setters when diffing objects for error (jestjs#9757)
  chore(jest-types): correct return type of shouldRunTestSuite fo… (jestjs#9753)
  ...
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants