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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing require.main inside of scripts loaded inside of isolateModules callback #9470

Closed
Alexsey opened this issue Jan 26, 2020 · 6 comments 路 Fixed by #10621
Closed

Missing require.main inside of scripts loaded inside of isolateModules callback #9470

Alexsey opened this issue Jan 26, 2020 · 6 comments 路 Fixed by #10621

Comments

@Alexsey
Copy link

Alexsey commented Jan 26, 2020

馃悰 Bug Report

If some script would be required inside of jest.isolateModules callback then require.main would be null inside of it. Thank is not how NodeJS works and not how the same script would work if it would be loaded outside of jest.isolateModules

To Reproduce

Steps to reproduce the behavior:

Consider a super simple script that would just use require.main.require index.js

const path = require('path')

module.exports = () => require.main.require(path.resolve(__dirname, './package.json'))

And a test for it

let foo
jest.isolateModules(() =>
  foo = require('./index')
)

test('test', () => {
  expect(foo()).toEqual({
    dependencies: { jest: '^25.1.0' },
    name: 'jest-main-require-bug',
    scripts: { test: 'jest' },
    version: '1.0.0'
  })
})

Expected behavior

The test should pass. And it will, if only 2 and 4 lines of index.test.js that wrap require inside of jest.isolateModules would be removed

Link to repl or repo (highly encouraged)

https://github.com/Alexsey/jest-require-main-bug

envinfo

npx: installed 1 in 3.765s

  System:
    OS: Windows 10 10.0.18362
    CPU: (4) x64 Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz
  Binaries:
    Node: 13.7.0 - C:\Program Files\nodejs\node.EXE
    npm: 6.13.6 - C:\Program Files\nodejs\npm.CMD
  npmPackages:
    jest: ^25.1.0 => 25.1.0
@SimenB
Copy link
Member

SimenB commented Jan 27, 2020

require.main is implemented by recursively walking up all .parent and returning the first one: https://github.com/facebook/jest/blob/084508290f4a90b28c3190021d3358ab1f753c3f/packages/jest-runtime/src/index.ts#L1018-L1033

See #5618 for some info on why this algorithm was chosen

Logic for parent: https://github.com/facebook/jest/blob/084508290f4a90b28c3190021d3358ab1f753c3f/packages/jest-runtime/src/index.ts#L756-L762

This breaks due to the way isolateModules is implemented, which is by making moduleRegistry a fake empty one: https://github.com/facebook/jest/blob/084508290f4a90b28c3190021d3358ab1f753c3f/packages/jest-runtime/src/index.ts#L533-L544

Followed by https://github.com/facebook/jest/blob/084508290f4a90b28c3190021d3358ab1f753c3f/packages/jest-runtime/src/index.ts#L330-L341

I'm not sure how to best untangle this so require.main is still set to the test module, but otherwise we ignore cached modules...

Would you be able to dig a bit into this an potentially provide a PR?

//cc @rogeliog in case you have any ideas

@jeysal
Copy link
Contributor

jeysal commented Feb 1, 2020

Looks like the same problem will exist with .parent?
I think we need to drop the "swapping out the module registry" approach and instead introduce an optional "first check the isolated module registry if it exists" somewhat similar to how we first check for mocks before real modules.
We can then enable that logic for certain types of lookups (like standard require) but not others (like .parent).

@SimenB
Copy link
Member

SimenB commented Feb 2, 2020

@jeysal yeah, agreed. The current approach is a bit too naive

@flozender
Copy link
Contributor

Can I help with this? Or does #10610 fix this?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2020

Yes please!

#10610 does not fix it since _isolatedModuleRegistry will still take preference. So we need to somehow keeping track of the main module. Possibly by assigning it as an instance variable mentioned here: #5618 (comment)

You solved the issue of "first require might not be test file" in #10610, so that should not be an issue

@github-actions
Copy link

This issue 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 a pull request may close this issue.

4 participants