feat: override Module#_compile #10072

skeggse · 2020-05-21T19:10:54Z

Hook into the jest module loading mechanisms for the Module#_compile functionality provided by Node. Fixes #10069.

Summary

Provides a consistent experience when interacting with uncommon features that load modules. See #10069.

Test plan

See unit tests.

This is probably incomplete; some of the edge cases of the semantics aren't clear to me, such as what should happen with the isolated registry when the require call happens outside the context of the isolateModules call.

jeysal

I've glanced over this without giving it a detailed review yet. Let's talk about the general approach first. IIUC you are making sure that the explicitly compiled module does not collide with a really existing file path by loading it in an isolated registry context? I think to avoid edge cases like the one you already predicted, a better approach would be having a code path that does not mutate the runtime state in the first place, which we can then use for _compile.

jeysal · 2020-05-21T20:20:55Z

packages/jest-runtime/src/__tests__/runtime_compile_module.test.js

+          'exports.value = 12;',
+          `${runtime.__mockRootPath}/dynamic.js`,
+        );
+        expect(module.exports).toEqual({value: 12});


Does this actually verify that the module was run in the runtime, with access to the environment etc.?

It really just exists to verify that it doesn't break basic functionality. The other test does verify that the module was run in the runtime because it verifies that jest is available. I do intend to add another test that checks that jsdom is present, just as soon as I figure out how to do that in this test setup.

packages/jest-runtime/src/index.ts

skeggse · 2020-05-22T17:44:39Z

I've glanced over this without giving it a detailed review yet. Let's talk about the general approach first. IIUC you are making sure that the explicitly compiled module does not collide with a really existing file path by loading it in an isolated registry context? I think to avoid edge cases like the one you already predicted, a better approach would be having a code path that does not mutate the runtime state in the first place, which we can then use for _compile.

Yeah I started with that approach, but realized that it wouldn't support certain cyclic module reference patterns like RegularModule.js -> ModuleWithSideEffects.js -> RegularModule.js. I suppose that might be out of scope for _compile'd source, but it might also be a feature that folks use? Moreover, it might be expected that loading ModuleWithSideEffects.js would load a new copy of the module in an isolated registry so that it doesn't interfere with the real modules, but the opposite might also be true. Seems probably best to require manual use of isolateModules, but that still doesn't help inform the original problem with requireing the module that was directly _compile'd. Thoughts?

jeysal · 2020-05-22T18:05:20Z

I would try to be like Node as much as possible. In Node,

x.js
---
module.exports = { a: 42 };
const module2 = new module.constructor.Module();
module2._compile("module.exports = require('./x.js')", "y.js");
console.log(module.exports);
console.log(module2.exports);
console.log(module.exports === module2.exports);

prints

{ a: 42 }
{ a: 42 }
true

So the compiled y module does get the same x module that was compiling it, just as it would with normal cyclic requires, and just as it would with cyclic requires in Jest. We want to achieve the same here.
Does a naive implementation with the approach not yield that behavior? I would think it does, since cyclic requires in Jest do behave the same as cyclic requires in Node.

skeggse · 2020-05-22T18:45:53Z

I would try to be like Node as much as possible.

I'm on board with that! I think I had a bad assumption about Node's default behavior and had been assuming that the new Module would end up linked from require.cache somehow. I definitely don't need that functionality, though, so I'll push the naive implementation. I do wonder if we'd like to support any manual modifications of the registry to support a pattern equivalent to:

import { Module } from 'module';

const m = new Module();
require.cache = id;
m._compile(fs.readFileSync(id, 'utf-8'), id);

So that if id referenced x.js that m.exports would equal { value: 'something' } in this example:

// x.js
require('./y');

// y.js
// This would mutate a new copy of x.js in the ModuleRegistry, rather than
// getting a reference to the x.js that loaded y.js.
require('./x.js').value = 'something';

skeggse · 2020-05-22T18:47:07Z

The only implementation detail I'm a little hung up on is the Module's filename property, which is null by default in Node, but can't be null in order to support the jest runtime's require functionality (and the current implementation of _execModule). The quick & dirty solution is to pass in a Proxy, I think, but that's not great. I could refactor the whole code path to support an alternate filename parameter?

EDIT: ok, put together a solution that seems reasonable; let me know if the filenameOverride doesn't belong in InternalModuleOptions and I can pull it out as a separate parameter.

Hook into the jest module loading mechanisms for the `Module#_compile` functionality provided by Node. Fixes jestjs#10069.

skeggse · 2020-05-22T19:43:30Z

Ready for review minus the appropriate tests for verifying test environment. Would love some guidance there - not sure how to control testEnvironment here.

skeggse · 2020-05-22T19:45:35Z

packages/jest-transform/src/ScriptTransformer.ts

+    // Skip cache for core and dynamically loaded modules.
+    if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {


The alternative here is to explicitly handle the case that the file doesn't exist when calling statSync in getScriptCacheKey, but it seemed best to just skip the cache for dynamically loaded modules.

Yeah def not the stat solution, there could still be a regular file for the path of the module that was compiled. Can we not use the regular filename arg and just flag it as dynamically compiled in the options though?

This also goes for all the filenameOverride ?? from.filename places - if we set the module's filename when it is compiled, can we not use that and just flag it as dynamically compiled?

Although I guess that would deviate from Node, where the filename is always null?

Yeah, that was my concern.

jeysal · 2020-05-24T19:59:15Z

Regarding require.cache modifications, the last discussion on that I've seen is in #9916

jeysal · 2020-05-24T20:15:27Z

packages/jest-runtime/src/index.ts

@@ -950,16 +957,17 @@ class Runtime {

  private _execModule(
    localModule: InitialModule,
+    moduleSource: string | undefined,


Name should probably indicate that this is only for modules explicitly compiled, not on the regular code path where the module is actually loaded from disk

How about runtimeModuleSource? dynamicModuleSource would work too but that starts to sound reminiscent of dynamic imports.

jeysal · 2020-05-24T20:22:13Z

packages/jest-transform/src/ScriptTransformer.ts

+    // Skip cache for core and dynamically loaded modules.
+    if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {


Yeah def not the stat solution, there could still be a regular file for the path of the module that was compiled. Can we not use the regular filename arg and just flag it as dynamically compiled in the options though?

jeysal · 2020-05-24T20:26:50Z

packages/jest-transform/src/ScriptTransformer.ts

+    // Skip cache for core and dynamically loaded modules.
+    if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {


This also goes for all the filenameOverride ?? from.filename places - if we set the module's filename when it is compiled, can we not use that and just flag it as dynamically compiled?

jeysal · 2020-05-24T20:27:14Z

packages/jest-transform/src/ScriptTransformer.ts

+    // Skip cache for core and dynamically loaded modules.
+    if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {


Although I guess that would deviate from Node, where the filename is always null?

…ompile

SimenB

Thanks! As a sanity check, could you add an integration test as well that works as you want using _compile and getting the correct env etc on the inside?

CHANGELOG.md

packages/jest-runtime/src/__tests__/runtime_compile_module.test.js

SimenB · 2020-07-28T11:38:34Z

packages/jest-runtime/src/index.ts

  private _execModule(
    localModule: InitialModule,
+    runtimeModuleSource: string | undefined,
    options: InternalModuleOptions | undefined,
    moduleRegistry: ModuleRegistry,
    from: Config.Path | null,
-  ) {
+  ): any | undefined {


private _execModule<T = unknown>( localModule: InitialModule, runtimeModuleSource: string | undefined, options: InternalModuleOptions | undefined, moduleRegistry: ModuleRegistry, from: Config.Path | null, ) { ): T | undefined {

This gets me an error from tsc -

packages/jest-runtime/src/index.ts:1062:7 - error TS2322: Type 'unknown' is not assignable to type 'T | undefined'. Type 'unknown' is not assignable to type 'T'. 'T' could be instantiated with an arbitrary type which could be unrelated to 'unknown'. 1062 return compiledFunction.call( ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1063 localModule.exports, ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ... 1079 }), ~~~~~~~~~~~ 1080 ); ~~~~~~~~

What do you recommend here?

do as T at the 1080 line to cast

SimenB · 2020-07-28T11:39:26Z

packages/jest-runtime/src/index.ts

    // should we implement the class ourselves?
-    class Module extends nativeModule.Module {}
+    class Module extends nativeModule.Module {
+      _compile(content: string, filename: string) {


Suggested change

_compile(content: string, filename: string) {

_compile(content: unknown, filename: unknown) {

I believe your typeofs narrow the type correctly.

I'm not really familiar with Typescript - I'm guessing that it'll infer the type based on type guards? This feels like a convention I'm not aware of - wouldn't we rather have the types be explicit for readability?

it's more honest since we get it from user input. And the typeofs will narrow it to string

SimenB · 2020-07-28T11:40:49Z

packages/jest-runtime/src/index.ts

+        }
+
+        return runtime._execModule(
+          this,


jest-runtime's internal InitialModule is not instanceof Module, I believe? It probably should, but if not (and it's hard to make it so) I think constructing a new object here rather than passing this makes sense

You're suggesting we make a fake Module instead, and provide that to the application code instead of leveraging the existing jest-runtime-specific instance? That seems reasonable to me, and might (?) fix the issues around filename and filenameOverride that @jeysal identified.

The current jest-runtime one should be Module ideally, yeah, then reused here. I think? 😅

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

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

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 · 2022-10-08T19:05:17Z

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 · 2022-11-08T00:10:37Z

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.

facebook-github-bot added the cla signed label May 21, 2020

skeggse mentioned this pull request May 21, 2020

Expose parts of runtime for dynamic module loading #10069

Closed

jeysal reviewed May 21, 2020

View reviewed changes

jeysal requested a review from SimenB May 21, 2020 20:28

skeggse force-pushed the override-module-compile branch 3 times, most recently from f3b9ae6 to 869f8f3 Compare May 22, 2020 19:12

feat: override Module#_compile

24938b2

Hook into the jest module loading mechanisms for the `Module#_compile` functionality provided by Node. Fixes jestjs#10069.

skeggse force-pushed the override-module-compile branch from 869f8f3 to 24938b2 Compare May 22, 2020 19:41

skeggse marked this pull request as ready for review May 22, 2020 19:43

skeggse commented May 22, 2020

View reviewed changes

jeysal reviewed May 24, 2020

View reviewed changes

Eli Skeggs added 4 commits July 15, 2020 18:36

chore: cleanup imports

2cb2b1c

refactor: rename moduleSource -> runtimeModuleSource

99c6879

test: exercise testEnvironment in compiled module

b1c49a3

Merge remote-tracking branch 'upstream/master' into override-module-c…

33bd363

…ompile

SimenB approved these changes Jul 28, 2020

View reviewed changes

skeggse and others added 5 commits July 29, 2020 11:49

chore: update changelog

8af8246

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

chore: remove redundant use strict directive

a670812

test: condense try-catch into expect.toThrow

e3b45ff

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

fix: use inferable _execModule signature

d08ca0a

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

Merge branch 'master' into override-module-compile

f58cc2e

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

github-actions bot closed this Oct 8, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: override Module#_compile #10072

feat: override Module#_compile #10072

skeggse commented May 21, 2020

jeysal left a comment

jeysal May 21, 2020

skeggse May 22, 2020

skeggse Jul 16, 2020

skeggse commented May 22, 2020

jeysal commented May 22, 2020

skeggse commented May 22, 2020

skeggse commented May 22, 2020 •

edited

Loading

skeggse commented May 22, 2020

skeggse May 22, 2020

jeysal May 24, 2020

jeysal May 24, 2020

jeysal May 24, 2020

skeggse Jul 16, 2020

jeysal commented May 24, 2020

jeysal May 24, 2020

skeggse Jul 16, 2020 •

edited

Loading

jeysal May 24, 2020

jeysal May 24, 2020

jeysal May 24, 2020

SimenB left a comment

SimenB Jul 28, 2020

skeggse Jul 29, 2020

SimenB Sep 14, 2020 •

edited

Loading

SimenB Jul 28, 2020

skeggse Jul 29, 2020

SimenB Jul 29, 2020

SimenB Jul 28, 2020

skeggse Jul 29, 2020 •

edited

Loading

SimenB Jul 29, 2020

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

github-actions bot commented Nov 8, 2022

		// Skip cache for core and dynamically loaded modules.
		if (!options.isCoreModule && typeof options.filenameOverride !== 'string') {

	_compile(content: string, filename: string) {
	_compile(content: unknown, filename: unknown) {

feat: override Module#_compile #10072

feat: override Module#_compile #10072

Conversation

skeggse commented May 21, 2020

Summary

Test plan

jeysal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skeggse commented May 22, 2020

jeysal commented May 22, 2020

skeggse commented May 22, 2020

skeggse commented May 22, 2020 • edited Loading

skeggse commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeysal commented May 24, 2020

Choose a reason for hiding this comment

skeggse Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skeggse Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

github-actions bot commented Nov 8, 2022

skeggse commented May 22, 2020 •

edited

Loading

skeggse Jul 16, 2020 •

edited

Loading

SimenB Sep 14, 2020 •

edited

Loading

skeggse Jul 29, 2020 •

edited

Loading