Fix isolateModules not working correctly when module has already been imported #8634

willym1 · 2019-07-03T05:50:57Z

Summary

This addresses #7863 where any requires inside isolateModules callback were retrieving modules from the regular module registry if the module was previously required in a non-isolated scope.

It didn't work previously because the logic checked for this._moduleRegistry.get(modulePath) FIRST to assign the variable as a regular module, which would always evaluate to true if the module had already been loaded.

The fix was to update the associated if block to check that this._internalModuleRegistry exists FIRST to assign the variable as an isolated module.

Test plan

Additional unit tests in runtime_require_module_or_mock.test.js
Linked jest-cli to my local sandbox project and ran additional tests (see below)

import incrementor from './incrementor';

describe('incrementor', () => {
  it('gets incrementor from the registry', () => {
    incrementor.increment()

    const cachedIncrementor = require('./incrementor').default

    expect(cachedIncrementor.getState()).toBe(2)
  })

  it('gets isolated incrementor', () => {
    let isolatedIncrementor

    jest.isolateModules(() =>
      isolatedIncrementor = require('./incrementor').default
    )
    expect(isolatedIncrementor.getState()).toBe(1)

    isolatedIncrementor = require('./incrementor').default
    expect(isolatedIncrementor.getState()).toBe(2)
  })

  it('can isolate the same module multiple times', () => {
    let isolatedIncrementor

    jest.isolateModules(() =>
      isolatedIncrementor = require('./incrementor').default
    )
    isolatedIncrementor.increment()

    jest.isolateModules(() =>
      isolatedIncrementor = require('./incrementor').default
    )
    expect(isolatedIncrementor.getState()).toBe(1)
  })
})

facebook-github-bot · 2019-07-03T05:51:06Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2019-07-03T06:00:03Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

codecov-io · 2019-07-03T06:06:17Z

Codecov Report

Merging #8634 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8634      +/-   ##
==========================================
- Coverage    63.4%   63.39%   -0.01%     
==========================================
  Files         274      274              
  Lines       11342    11341       -1     
  Branches     2770     2769       -1     
==========================================
- Hits         7191     7190       -1     
  Misses       3534     3534              
  Partials      617      617

Impacted Files	Coverage Δ
packages/jest-runtime/src/index.ts	`68.96% <100%> (-0.09%)`	⬇️

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 2b64bb4...1ae4569. Read the comment docs.

willym1 · 2019-07-03T06:26:07Z

This only updates behavior with regular modules and not mocked modules. The field name _isolatedMockRegistry exists and there's similar code inside requireMock method but I left it alone since I'm not even sure what the use case would be for isolating mocked modules

thymikee

LGTM. Cc @rogeliog

rogeliog · 2019-07-04T17:52:42Z

This only updates behavior with regular modules and not mocked modules. The field name _isolatedMockRegistry exists and there's similar code inside requireMock method but I left it alone since I'm not even sure what the use case would be for isolating mocked modules

From what I understand the use case for this is for whenever you are requiring a module that is mocked, which could be stateful. In some cases that mock might have been declared outside of the test file. For example in a setupFilesAfterEnv or moduleNameMapper

rogeliog · 2019-07-04T17:47:21Z

packages/jest-runtime/src/index.ts

-      ) {
-        moduleRegistry = this._moduleRegistry;
-      } else {
+      if (this._isolatedModuleRegistry) {


For the most part it looks good, but there is one thing that worries me. With this new implementation there is one change in behavior.

Let's say that I have module A which depends in module B. If I do . jest.isolateModules(() => A = require('./A');) it will now also use a different copy of "B" also_(as well as for the dependencies of B)_ I'm worried that this might not be the what the user expects, and might make it a bit hard to debug issues.

My example of A and B might be easier to understand with A being MyComponent and B being React.

thoughts? @SimenB @thymikee

Good point. I've updated the code to not affect dependencies within an isolated module

Hi!

Thanks for all the work. Sorry, what I meant with my comment was just to express that there would be a change in behavior, I’m not saying that one is right and the other isn’t.

@thymikee @SimenB @willym1 I think it is important define exactly how we want isolateModules to work before proceeding.

I see a couple of options:

[Current behavior] When an isolateModules block gets executed, only modules that are not in the module registry yet will be isolated. This means that things that have already been required will use the existing “require”.

When an isolateModules block gets executed, it isolates every single module that gets required.

When an isolateModules block gets executed, only the required only the direct requires get isolated but not their internal dependencies.

Here are thoughts for the each option:

Rolling out option 2 and 3 will break a lot of existing tests when people upgrade their versions of Jest in ways that might be hard to spot.

Option 1 still has the problem mentioned in Requiring modules with isolateModules does not work if module has already been required with a non-isolated method e.g: using require. #7863, but for some reason I still I feel this might be the safest behavior for the user(as well as for maintainability). I wonder if we could log a warning when we detect that the user is trying to require a module that is already in the registry. I’m not sure if we can correctly identify those cases without much false positives.

What are your thoughts? @thymikee @SimenB @willym1

This means that things that have already been required will use the existing “require”.

This is not really documented, isn't it?

I think I lean towards 2, since I'd expect everything inside the callback to get its own copy. This is fairly fresh addition to Jest, so I think there won't be too much cruft around it, especially if we're fixing it for good reasons.

This is not really documented, isn't it?

Correct, it is not documented.

Leaning towards 2 as well. 1 doesn't seem functionally intuitive. If you needed to do multiple tests on a module that requires each one to be a fresh copy, you'd have to individually isolate each module at the start of the test and know beforehand how many copies are needed.

1 and 3 suffers from another issue - since isolateModules calls aren't hoisted like a regular mock call would (correct me on this if I'm wrong), there's no guarantee that the internal dependencies of those copies were already previously required. Say you were isolating a module multiple times whose behavior is tied to some stateful internal dependencies. Any copy of the isolated module can indirectly affect the other copies simply by running, which may be problematic if you're trying to do multiple isolated tests. This is just a very specific case btw, I don't see this significantly impacting anyone currently using isolatedModules in their tests.

@rogeliog In my opinion, option 2 is the most intuitive, and the only behaviour that matches the name of the function. This is exactly what I thought isolatedModules did when I read the documentation, especially after having just read the documentation for resetModules() which is right above it, and the explicit mention of the word "sandbox".

This is just a very specific case btw, I don't see this significantly impacting anyone currently using isolatedModules in their tests.

@willym1 I found this issue+PR because this is something I'm currently struggling with. Because of this bug (and because of the lack of async import support: #10428) I've had to use resetModules() instead.

willym1 · 2019-07-05T11:19:27Z

Made some revisions addressing @rogeliog's comments:

Accommodate isolating mocked modules
Dependencies being imported within an isolated module won't be indirectly isolated. This is accomplished by adding a new boolean field _isolatedRequireInProgress to the Runtime class. Its purpose is to keep track of the current isolated module being loaded. If the value is set to true, avoid isolating any modules. A local variable disableIsolatedRequireInProgress within requireModule and requireMock signal when to begin isolating again.

(also idk what happened here, a bunch of checks just failed on a single test in jest-util. I'm without context but it seems unrelated to my changes)

SimenB · 2019-07-05T12:12:57Z

Not sure about CI... Master just passed, would you mind rebasing. Maybe circle was weird for a moment?

willym1 · 2019-07-05T20:59:15Z

Seems like the test failure's reproducible with my code. Ran the specific test locally and passed. Not sure what to do about this one 😕

willym1 · 2019-07-07T03:18:25Z

@rogeliog Ready for re-review

SimenB · 2019-11-09T16:22:11Z

@willym1 could you rebase this, please?

helielson · 2019-11-30T12:50:16Z

Any updates here? @willym1 do you need any help?

Ranguna · 2020-05-01T17:36:49Z

No updates since November. What's going on here ?

SimenB · 2020-05-01T18:36:38Z

Just waiting for a rebase, I think it's good to land? Maybe @rogeliog wants to give it another lookover?

willym1 · 2020-05-01T19:06:12Z

Hi - sorry this had been sitting for a while and I've forgotten about it. Last time I checked it seems there was still some unresolved implementation discussion.

Will hop on this PR again this weekend and see whether there's additional changes needed on my latest update and try to merge if everything's good to go.

Ranguna · 2020-05-02T13:31:56Z

Good to see that you're all still working on this.

Thank you for all the good work 👍

SimenB · 2020-05-05T12:08:10Z

@willym1 seems like this broke a test in packages/jest-regex-util, could you take a look? Whatever the regression there was, we should add a dedicated test for it

willym1 · 2020-05-05T13:30:15Z

Yeah there's an issue with how isolated modules deals with mocks. Will investigate some more

SimenB · 2020-10-19T10:43:36Z

@willym1 were you able to look more into this?

github-actions · 2022-09-08T18:56:54Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

Ranguna · 2022-09-09T07:42:31Z

I'm unsure if this has been fixed by another PR, but I'll just comment here to prevent bot supremacy.

SimenB · 2022-09-09T08:07:18Z

I've merged in main and resolved the conflicts. That said, I don't know the state of this. @willym1 are you still up for finishing this? 🙂

SimenB · 2022-09-09T08:09:07Z

#10963 seems to have addressed the same issue? The test added in this PR doesn;t pass on master however, while the test added in that PR still passes. Not sure if the behaviour here is more correct, tho 😅

ANd regardless, we need to fix the error in the regex tests

github-actions · 2023-09-09T09:04:06Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2023-10-09T09:04:40Z

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions · 2023-11-09T00:08:10Z

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.

willym1 changed the title ~~Fix isolateModules not working correctly when module has already been imported~~ Fix isolateModules not working correctly when module has already been imported Jul 3, 2019

facebook-github-bot added the cla signed label Jul 3, 2019

SimenB requested a review from rogeliog July 3, 2019 06:07

willym1 marked this pull request as ready for review July 3, 2019 06:27

thymikee approved these changes Jul 3, 2019

View reviewed changes

rogeliog reviewed Jul 4, 2019

View reviewed changes

willym1 force-pushed the isolateModules-fix branch 2 times, most recently from 15d7d5d to 96a19a1 Compare July 5, 2019 10:36

willym1 force-pushed the isolateModules-fix branch from 96a19a1 to 2174d5a Compare July 5, 2019 20:30

willym1 force-pushed the isolateModules-fix branch from c28f8b1 to c221ac7 Compare July 7, 2019 02:54

byCedric mentioned this pull request Apr 13, 2020

[expo-auth-session] Fix static manifest references expo/expo#7822

Closed

willym1 force-pushed the isolateModules-fix branch from c221ac7 to 77ecb1a Compare May 5, 2020 01:28

willym1 added 2 commits May 5, 2020 00:02

Fix isolateModules not working when module has already been imported

95e9b37

Update CHANGELOG.md

bf0e82f

willym1 force-pushed the isolateModules-fix branch from a8338b8 to bf0e82f Compare May 5, 2020 04:05

SimenB added 2 commits May 5, 2020 13:26

Merge branch 'master' into isolateModules-fix

e5cf5c4

move changelog entry

e930c37

ahnpnl mentioned this pull request Sep 18, 2020

jest isolatemodules are not working as expected kulshekhar/ts-jest#1964

Closed

github-actions bot added the Stale label Sep 8, 2022

github-actions bot removed the Stale label Sep 9, 2022

Merge branch 'main' into isolateModules-fix

9491352

eslint

5cc196b

github-actions bot added the Stale label Sep 9, 2023

github-actions bot closed this Oct 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix isolateModules not working correctly when module has already been imported #8634

Fix isolateModules not working correctly when module has already been imported #8634

willym1 commented Jul 3, 2019 •

edited

Loading

facebook-github-bot commented Jul 3, 2019

facebook-github-bot commented Jul 3, 2019

codecov-io commented Jul 3, 2019 •

edited

Loading

willym1 commented Jul 3, 2019

thymikee left a comment

rogeliog commented Jul 4, 2019

rogeliog Jul 4, 2019 •

edited

Loading

willym1 Jul 5, 2019

rogeliog Jul 8, 2019

thymikee Jul 9, 2019

rogeliog Jul 9, 2019

willym1 Jul 11, 2019 •

edited

Loading

vvanpo Aug 20, 2020

willym1 commented Jul 5, 2019

SimenB commented Jul 5, 2019

willym1 commented Jul 5, 2019 •

edited

Loading

willym1 commented Jul 7, 2019

SimenB commented Nov 9, 2019

helielson commented Nov 30, 2019

Ranguna commented May 1, 2020

SimenB commented May 1, 2020

willym1 commented May 1, 2020

Ranguna commented May 2, 2020

SimenB commented May 5, 2020

willym1 commented May 5, 2020

SimenB commented Oct 19, 2020

github-actions bot commented Sep 8, 2022

Ranguna commented Sep 9, 2022

SimenB commented Sep 9, 2022

SimenB commented Sep 9, 2022 •

edited

Loading

github-actions bot commented Sep 9, 2023

github-actions bot commented Oct 9, 2023

github-actions bot commented Nov 9, 2023

Fix isolateModules not working correctly when module has already been imported #8634

Fix isolateModules not working correctly when module has already been imported #8634

Conversation

willym1 commented Jul 3, 2019 • edited Loading

Summary

Test plan

facebook-github-bot commented Jul 3, 2019

facebook-github-bot commented Jul 3, 2019

codecov-io commented Jul 3, 2019 • edited Loading

Codecov Report

willym1 commented Jul 3, 2019

thymikee left a comment

Choose a reason for hiding this comment

rogeliog commented Jul 4, 2019

rogeliog Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

willym1 Jul 5, 2019

Choose a reason for hiding this comment

rogeliog Jul 8, 2019

Choose a reason for hiding this comment

thymikee Jul 9, 2019

Choose a reason for hiding this comment

rogeliog Jul 9, 2019

Choose a reason for hiding this comment

willym1 Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

vvanpo Aug 20, 2020

Choose a reason for hiding this comment

willym1 commented Jul 5, 2019

SimenB commented Jul 5, 2019

willym1 commented Jul 5, 2019 • edited Loading

willym1 commented Jul 7, 2019

SimenB commented Nov 9, 2019

helielson commented Nov 30, 2019

Ranguna commented May 1, 2020

SimenB commented May 1, 2020

willym1 commented May 1, 2020

Ranguna commented May 2, 2020

SimenB commented May 5, 2020

willym1 commented May 5, 2020

SimenB commented Oct 19, 2020

github-actions bot commented Sep 8, 2022

Ranguna commented Sep 9, 2022

SimenB commented Sep 9, 2022

SimenB commented Sep 9, 2022 • edited Loading

github-actions bot commented Sep 9, 2023

github-actions bot commented Oct 9, 2023

github-actions bot commented Nov 9, 2023

willym1 commented Jul 3, 2019 •

edited

Loading

codecov-io commented Jul 3, 2019 •

edited

Loading

rogeliog Jul 4, 2019 •

edited

Loading

willym1 Jul 11, 2019 •

edited

Loading

willym1 commented Jul 5, 2019 •

edited

Loading

SimenB commented Sep 9, 2022 •

edited

Loading