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

Jump to bottom

feat: override module.createRequire #9469

Merged
merged 13 commits into from Feb 1, 2020
Merged

feat: override module.createRequire #9469

merged 13 commits into from Feb 1, 2020

Conversation

doniyor2109
Copy link
Contributor

PR for #9426

@codecov-io
Copy link

codecov-io commented Jan 26, 2020 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@af97309). Click here to learn what that means.
The diff coverage is 86.36%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #9469   +/-   ##
========================================
  Coverage          ?     65%           
========================================
  Files             ?     283           
  Lines             ?   12131           
  Branches          ?    3002           
========================================
  Hits              ?    7886           
  Misses            ?    3604           
  Partials          ?     641
Impacted Files Coverage Δ
packages/pretty-format/src/index.ts 95.03% <ø> (ø)
packages/jest-util/src/createDirectory.ts 0% <0%> (ø)
packages/jest-snapshot/src/utils.ts 95.6% <100%> (ø)
packages/jest-runtime/src/index.ts 65.22% <100%> (ø)
packages/jest-reporters/src/coverage_reporter.ts 51.21% <100%> (ø)
packages/jest-transform/src/ScriptTransformer.ts 69.64% <84.61%> (ø)

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 af97309...ecce093. Read the comment docs.

SimenB
SimenB reviewed Jan 26, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

nice, thank you! I think we should add some more validation about the shape of the passed in path, other than that this lgtm

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
SimenB
SimenB approved these changes Feb 1, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is great, thank you!

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
yarn.lock Outdated
resolved "https://registry.yarnpkg.com/@types/node/-/node-13.7.0.tgz#b417deda18cf8400f278733499ad5547ed1abec4"
integrity sha512-GnZbirvmqZUzMgkFn70c74OQpTTUcCzlhQliTzYjQMqg+hVKcDnxdL19Ne3UdYzdMA/+W3eb646FWn/ZaT1NfQ==

"@types/node@>= 8":
Copy link
Member

Choose a reason for hiding this comment

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

roll back change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have upgraded @types/node to 13.7 because this version includes syncBuiltinESMExports in module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which I have used here
https://github.com/facebook/jest/pull/9469/files#diff-5b473ffb7e91f9fc070a6f6025790ef0R918

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reverted changes back. I just used another type

Copy link
Member

Choose a reason for hiding this comment

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

oh! I just didn't want 2 versions of it, could have deduped it to the newer ones. Lemme play with it locally. Thanks!

@SimenB SimenB mentioned this pull request Feb 1, 2020
Open
21 tasks
@SimenB SimenB changed the title Origin/override module create require feat: override module.createRequire Feb 1, 2020
@SimenB SimenB merged commit 523ba3b into jestjs:master Feb 1, 2020
@SimenB
Copy link
Member

SimenB commented Feb 1, 2020

Thank you so much @doniyor2109!

@SimenB SimenB mentioned this pull request Feb 1, 2020
Closed
This was referenced Mar 26, 2020
Closed
Merged
This was referenced Apr 16, 2020
Open
Open
Merged
Closed
Merged
This was referenced Apr 25, 2020
Merged
Closed
@snyk-bot snyk-bot mentioned this pull request May 4, 2020
Closed
@SimenB SimenB mentioned this pull request Jun 20, 2020
Merged
sodatea added a commit to vuejs/vue-cli that referenced this pull request Jan 20, 2021
@sodatea
test: simplify loadModule special case handling for tests
82aef12 
1. createRequire should have been properly handled in newer Jest
versions: jestjs/jest#9469
2. We don't test migrators with Jest mock modules anymore

So it's safe to skip that condition
@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.
Reviewers

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@doniyor2109 @codecov-io @SimenB @facebook-github-bot