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

fix: add require.main when using isolateModules #10621

Merged
merged 6 commits into from Oct 11, 2020

Conversation

flozender
Copy link
Contributor

@flozender flozender commented Oct 11, 2020

Summary

Aims to close #9470.

Modules required through jest.isolateModules() do not have a require.main property on them.

I have attached a mainModule variable to the runtime instance. It will be null at first, and the first module seen will be assigned to the mainModule variable. Required modules will get the mainModule attached to them as main.

Test plan

Emulate the scenario from the issue as a test.

@flozender flozender marked this pull request as draft October 11, 2020 14:01
@flozender flozender changed the title Add mainModule instance variable to runtime fix: add mainModule instance variable to runtime Oct 11, 2020
@flozender flozender marked this pull request as ready for review October 11, 2020 15:38
@flozender flozender requested a review from SimenB October 11, 2020 15:38
@SimenB SimenB changed the title fix: add mainModule instance variable to runtime fix: add require.main when using isolateModules Oct 11, 2020
@SimenB SimenB merged commit c41420f into jestjs:master Oct 11, 2020
@SimenB
Copy link
Member

SimenB commented Oct 11, 2020

Thanks again, @flozender!

@jsg2021
Copy link

jsg2021 commented Oct 11, 2020

In 26.5.3 require.main is undefined in general for me... works fine in 26.4.2. I'm not using isolateModules.

@jsg2021
Copy link

jsg2021 commented Oct 11, 2020

I should clarify, in the test file require.main is populated, but in code that my tests require() in, there the require.main is undefined.

[Function: bound requireModuleOrMock] {
  extensions: [Object: null prototype] {},
  resolve: [Function: resolve] { paths: [Function (anonymous)] },
  cache: [Object: null prototype] {},
  main: undefined
}

@jsg2021
Copy link

jsg2021 commented Oct 11, 2020

Here is a code sandbox reproduction:
https://codesandbox.io/s/gifted-sara-x25fr

@flozender
Copy link
Contributor Author

flozender commented Oct 12, 2020

@jsg2021 I'm able to run it with no issues locally, I don't know what's happening on CodeSandbox. Have you tried running it locally?

image

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

I created it after local tests started failing.

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

Are there environment vars in your local env set that effect jest?

@flozender
Copy link
Contributor Author

No, I don't think so.

@flozender
Copy link
Contributor Author

flozender commented Oct 12, 2020

@jsg2021 Looks like CodeSandbox doesn't work on v26.4.2 either: https://codesandbox.io/s/thirsty-rgb-pt0xf

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

I have had a really hard time pinning jest to any minor version... the jest library will pin, but it appears all its dependencies use ^ so they will all install the 26.5.3. To replicate, we would have to specify all jest's dependencies with a pin.

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

@flozender So, its still failing because all the jest dependencies of the jest package are at 26.5.3, not 26.4.2

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

Interesting. I downloaded the code from the sandbox and ran it, passed... so that made me compare configs, and I found the code fails if I add this to the package.json:

"jest": {
	"resetModules": true
}

@flozender
Copy link
Contributor Author

Interesting, maybe you should open an issue for this!

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

My pre-existing tests still pass on 26.4.2 (I have a package-lock) but fail on 26.5.3. My jest config does not enable that flag.

@flozender
Copy link
Contributor Author

Can confirm it fails on using "resetModules": true. I can take up the issue once you open it. Let me know!

@flozender
Copy link
Contributor Author

@jsg2021 here's a sample repo you can add to the issue: https://github.com/flozender/jest-resetModules

@jsg2021
Copy link

jsg2021 commented Oct 12, 2020

Done! #10625

@SimenB
Copy link
Member

SimenB commented Oct 12, 2020

@jsg2021 are you able to put together a repository showing your failure? Even if it's a work project you should be able to copy over the lockfile and just delete everything not jest from package.json then run npm or yarn and have the lockfile keep the jest packages while removing everything else. then just stick a simple test in there that shows the failure

@jsg2021
Copy link

jsg2021 commented Oct 13, 2020

Ah! thanks for the tip @SimenB! Here is a repo that exemplifies my passing state:
https://github.com/jsg2021/jest-require-main-issue

Side note: I figured out why it looked like I had resetModules: true... my test suite had a beforeEach calling jest.resetModules()...

Anyways... the test cases we were trying to make before were directly importing the module... I didn't think that mattered, so I didn't think anything of it, but I added a redirection layer and the tests started passing for that... they still fail for the direct case and probably shouldn't... so if you delete the lock and install latest, all tests fail.

@SimenB
Copy link
Member

SimenB commented Oct 13, 2020

Thanks @jsg2021. Does that mean it was covered by #10625?

@jsg2021
Copy link

jsg2021 commented Oct 13, 2020

@SimenB I think so. That repo will reproduce my exact issue where it was passing with the lock file, but broken when updating. It also has the original attempt to reproduce the issue where we simply wrote a test that inspected require.main directly... which was always broken with resetModules:true/jest.resetModules(). However, my project had one module require in between the test and the code referencing require.main...and that was working until I updated. I believe the fix for #10625 will resolve it, but I do think the additional test cases should be added.

@SimenB
Copy link
Member

SimenB commented Oct 13, 2020

Sounds good, thanks! @flozender wanna send a PR for the extra test cases? 😀

@flozender
Copy link
Contributor Author

flozender commented Oct 13, 2020

@SimenB @jsg2021 I went ahead and added tests for both the variations of resetModules in #10626 before we merged the PR! :)

@jsg2021
Copy link

jsg2021 commented Oct 13, 2020

@flozender I saw that, but the indirect case is missing for each variation.

@jsg2021
Copy link

jsg2021 commented Oct 13, 2020

The indirect case is where the test verifies that requireFromMain works indirectly... so it's not being directly imported by the test.

@flozender
Copy link
Contributor Author

Oh, I see. I'll send in a PR shortly, thanks.

@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.

Missing require.main inside of scripts loaded inside of isolateModules callback
4 participants