Add extraGlobals option
#7454
Add extraGlobals option
#7454
Conversation
corevo
force-pushed
the
environment-local-lookup
branch
from
38918f0
to
03a2e14
Compare
|
We should probably make the environment be part of the cache key; otherwise changing the environment via config would make the transform cache return stale results. |
|
I wonder if it makes sense to inject all Node/JS globals like that by default? |
|
How does this work with sourcemaps, and thus stack traces and coverage? If it's an issue, can we use Babel to wrap the code instead (adding it to |
|
@SimenB that's a great point, I'm adding that before the transpilation, which means it would be added to the sourcemaps. |
|
What about something like this (untested) approach? diff --git i/packages/jest-runtime/src/index.js w/packages/jest-runtime/src/index.js
index f91e481a2..f88d61e2e 100644
--- i/packages/jest-runtime/src/index.js
+++ w/packages/jest-runtime/src/index.js
@@ -617,12 +617,14 @@ class Runtime {
value: this._createRequireImplementation(localModule, options),
});
+ const extraGlobals = ['Math'];
const transformedFile = this._scriptTransformer.transform(
filename,
{
collectCoverage: this._coverageOptions.collectCoverage,
collectCoverageFrom: this._coverageOptions.collectCoverageFrom,
collectCoverageOnlyFrom: this._coverageOptions.collectCoverageOnlyFrom,
+ extraGlobals,
isInternalModule,
},
this._cacheFS[filename],
@@ -670,6 +672,7 @@ class Runtime {
// $FlowFixMe
(localModule.require: LocalModuleRequire),
), // jest object
+ ...extraGlobals.map(thing => global[thing]),
);
this._isCurrentlyExecutingManualMock = origCurrExecutingManualMock;
diff --git i/packages/jest-runtime/src/script_transformer.js w/packages/jest-runtime/src/script_transformer.js
index a25683156..66c88e6f8 100644
--- i/packages/jest-runtime/src/script_transformer.js
+++ w/packages/jest-runtime/src/script_transformer.js
@@ -38,6 +38,7 @@ export type Options = {|
collectCoverageOnlyFrom: ?{[key: string]: boolean, __proto__: null},
isCoreModule?: boolean,
isInternalModule?: boolean,
+ extraGlobals?: Array<string>,
|};
type ProjectCache = {|
@@ -305,6 +306,8 @@ export default class ScriptTransformer {
(this._shouldTransform(filename) || instrument);
try {
+ const extraGlobals = options.extraGlobals || [];
+
if (willTransform) {
const transformedSource = this.transformSource(
filename,
@@ -312,11 +315,11 @@ export default class ScriptTransformer {
instrument,
);
- wrappedCode = wrap(transformedSource.code);
+ wrappedCode = wrap(transformedSource.code, ...extraGlobals);
sourceMapPath = transformedSource.sourceMapPath;
mapCoverage = transformedSource.mapCoverage;
} else {
- wrappedCode = wrap(content);
+ wrappedCode = wrap(content, ...extraGlobals);
}
return {
@@ -517,11 +520,25 @@ const calcIgnorePatternRegexp = (config: ProjectConfig): ?RegExp => {
return new RegExp(config.transformIgnorePatterns.join('|'));
};
-const wrap = content =>
- '({"' +
- ScriptTransformer.EVAL_RESULT_VARIABLE +
- '":function(module,exports,require,__dirname,__filename,global,jest){' +
- content +
- '\n}});';
+const wrap = (content, ...extras) => {
+ const args = [
+ 'module',
+ 'exports',
+ 'require',
+ '__dirname',
+ '__filename',
+ 'global',
+ 'jest',
+ ...extras,
+ ];
+
+ return (
+ '({"' +
+ ScriptTransformer.EVAL_RESULT_VARIABLE +
+ `":function(${args.join(',')}){` +
+ content +
+ '\n}});'
+ );
+};
ScriptTransformer.EVAL_RESULT_VARIABLE = 'Object.<anonymous>';
|
|
Doing that to the reproduction in #5163 results in this: So it seems to work, and doesn't require a custom env. It also reuses the wrapping we already do, so no extra wrapping not already present. |
|
I was thinking of using the environment in case you want to initialize something and pass that as a global as well. |
|
I think it's a more correct change. People can still make their own |
corevo
force-pushed
the
environment-local-lookup
branch
2 times, most recently
from
321110b
to
830557c
Compare
Codecov Report
@@ Coverage Diff @@
## master #7454 +/- ##
==========================================
- Coverage 67.47% 66.79% -0.69%
==========================================
Files 247 242 -5
Lines 9514 9382 -132
Branches 5 5
==========================================
- Hits 6420 6267 -153
- Misses 3092 3113 +21
Partials 2 2
Continue to review full report at Codecov.
|
|
I went with the direction you were explaining, if you like it, I'll tidy it up with tests and docs. |
There was a problem hiding this comment.
This needs to be added to https://jestjs.io/docs/en/configuration
| @@ -26,6 +26,7 @@ const DEFAULT_GLOBAL_CONFIG: GlobalConfig = { | |||
| enabledTestsMap: null, | |||
| errorOnDeprecated: false, | |||
| expand: false, | |||
| extraGlobals: [], | |||
There was a problem hiding this comment.
@rickhanlonii thoughts on naming here?
corevo
force-pushed
the
environment-local-lookup
branch
2 times, most recently
from
fc2f25b
to
3dd3c87
Compare
corevo
force-pushed
the
environment-local-lookup
branch
from
3dd3c87
to
5a61fe5
Compare
corevo
force-pushed
the
environment-local-lookup
branch
from
5a61fe5
to
e986629
Compare
|
@rickhanlonii gonna merge, feel free to yell at me if you don't like the name 😀 |
SimenB
changed the title
extraGlobals option
|
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. |
Summary
This diffs aims to give the user a workaround for expensive vm property lookups.
As explained here, lookups for undefined properties inside the vm context are expensive, as node will go and try to look them up in the sandbox and then further up in the global context, each of those is very expensive.
I have tests that constantly make calls to
Math, and these lookups render them horrendously slow, the problem fixing the issue, is that theres plenty of different variables, unknown to jest which ones will be needed, and in fact, this is a node problem.But still we gotta let the users have a way to get tests to be fast.
The idea is to allow users define these variables through an optional method in the Environment, for the case of #5163 it would be like so:
Fixes #5163
Test plan
I ran
yarn test, seems to fail due to unrelated unstable changes in master.Also wanna know what's the best way to test this change, and wether this change should be reflected here.