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

fix: require.main undefined with createRequire() #10610

Merged
merged 24 commits into from Oct 10, 2020
Merged

fix: require.main undefined with createRequire() #10610

merged 24 commits into from Oct 10, 2020

Conversation

flozender
Copy link
Contributor

@flozender flozender commented Oct 8, 2020 •
edited

Summary

Closes #10479.

The require.main of modules that are required from within a test file is undefined, when:

  • those modules were required with a require created with createRequire
  • createRequire was given another module than the current one
  • the test file is run with Jest

This PR aims to fix this issue.

Test plan

Integration test to check if the scenario from the issue works as expected.

@flozender flozender marked this pull request as draft October 8, 2020 17:37
SimenB
SimenB requested changes Oct 8, 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.

Thanks! For a test, possibly add the example from the issue as an integration test, asserting that the path is correct?

packages/jest-runner/src/runTest.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
@flozender
Copy link
Contributor Author

Thanks for the review, will get on the test now.

flozender and others added 3 commits October 9, 2020 14:59
@flozender flozender requested a review from SimenB October 9, 2020 13:45
@flozender flozender marked this pull request as ready for review October 9, 2020 16:42
jeysal
jeysal reviewed Oct 9, 2020
View reviewed changes
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

PR overall looks good!
Commented with one possible improvement on the test

e2e/require-main-after-create-require/__tests__/parent.test.js Outdated Show resolved Hide resolved
@flozender flozender requested a review from jeysal October 10, 2020 04:31
SimenB
SimenB approved these changes Oct 10, 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.

Thanks!

e2e/__tests__/requireMainAfterCreateRequire.test.ts Outdated Show resolved Hide resolved
e2e/__tests__/requireMainAfterCreateRequire.test.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
e2e/require-main-after-create-require/__tests__/parent.test.js Outdated Show resolved Hide resolved
flozender and others added 3 commits October 10, 2020 15:42
@flozender @SimenB
change min node version
e7d7c6c 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender @SimenB
Update e2e/require-main-after-create-require/__tests__/parent.test.js
1119acf 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender
rm path.resolve() and update changelog
fe09bdb
@flozender flozender requested a review from SimenB October 10, 2020 11:06
SimenB
SimenB approved these changes Oct 10, 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.

miniscule nits, this LGTM

e2e/__tests__/requireMainAfterCreateRequire.test.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
flozender and others added 4 commits October 10, 2020 16:56
@flozender @SimenB
Update e2e/__tests__/requireMainAfterCreateRequire.test.ts
aa4c4a8 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender @SimenB
Update CHANGELOG.md
e410eb7 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender @SimenB
Update packages/jest-runtime/src/index.ts
8c57b50 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender @SimenB
Update packages/jest-runtime/src/index.ts
e4edca5 
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@flozender flozender changed the title fix: require.main undefined with createRequire() fix: require.main undefined with createRequire() Oct 10, 2020
@flozender flozender requested a review from SimenB October 10, 2020 11:35
@SimenB SimenB merged commit acd7c83 into jestjs:master Oct 10, 2020
@flozender
Copy link
Contributor Author

Thanks for the great help! @SimenB @jeysal

@flozender flozender mentioned this pull request Oct 11, 2020
Closed
@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

@jeysal jeysal Awaiting requested review from jeysal

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

Successfully merging this pull request may close these issues.

require.main undefined with createRequire()
4 participants
@flozender @SimenB @jeysal @facebook-github-bot