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.

Sign up for GitHub

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

feat: pass ESM options to transformers #9597

Merged
merged 1 commit into from
Apr 5, 2020
+76 −17
Merged

feat: pass ESM options to transformers #9597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Features

- `[babel-jest]` Support passing `supportsDynamicImport` and `supportsStaticESM` ([#9766](https://github.com/facebook/jest/pull/9766))
- `[jest-runtime, @jest/transformer]` Support passing `supportsDynamicImport` and `supportsStaticESM` ([#9597](https://github.com/facebook/jest/pull/9597))

### Fixes

Expand Down
16 changes: 14 additions & 2 deletions packages/jest-runtime/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ type HasteMapOptions = {

type InternalModuleOptions = {
isInternalModule: boolean;
supportsDynamicImport: boolean;
supportsStaticESM: boolean;
};

const defaultTransformOptions: InternalModuleOptions = {
isInternalModule: false,
supportsDynamicImport: false,
supportsStaticESM: false,
};

type InitialModule = Partial<Module> &
Expand Down Expand Up @@ -368,7 +376,11 @@ class Runtime {
}

requireInternalModule<T = unknown>(from: Config.Path, to?: string): T {
return this.requireModule(from, to, {isInternalModule: true});
return this.requireModule(from, to, {
isInternalModule: true,
supportsDynamicImport: false,
supportsStaticESM: false,
});
}

requireActual<T = unknown>(from: Config.Path, moduleName: string): T {
Expand Down Expand Up @@ -493,7 +505,7 @@ class Runtime {
}

private _getFullTransformationOptions(
options: InternalModuleOptions | undefined,
options: InternalModuleOptions = defaultTransformOptions,
): TransformationOptions {
return {
...options,
Expand Down
76 changes: 61 additions & 15 deletions packages/jest-transform/src/ScriptTransformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ export default class ScriptTransformer {
fileData: string,
filename: Config.Path,
instrument: boolean,
supportsDynamicImport: boolean,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made them non-optional for all private methods and default false for the public ones

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should ideally use options objects, as 2-3 boolean flag inputs are quite confusing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally agree, but IIRC functions with more arguments are optimized better than the ones with object arguments, and this is kinda hot path

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentially we could add separate functions? the es module path can be async (or rather, always is) as well, so that might tie nicely into #9504. Then we could ditch the supportsDynamicImport option since that works for both esm and cjs (and can be async) and just vary on the static version

supportsStaticESM: boolean,
): string {
const configString = this._cache.configString;
const transformer = this._getTransformer(filename);
Expand All @@ -101,8 +103,8 @@ export default class ScriptTransformer {
config: this._config,
instrument,
rootDir: this._config.rootDir,
supportsDynamicImport: false,
supportsStaticESM: false,
supportsDynamicImport,
supportsStaticESM,
}),
)
.update(CACHE_VERSION)
Expand All @@ -122,13 +124,21 @@ export default class ScriptTransformer {
filename: Config.Path,
content: string,
instrument: boolean,
supportsDynamicImport: boolean,
supportsStaticESM: boolean,
): Config.Path {
const baseCacheDir = HasteMap.getCacheFilePath(
this._config.cacheDirectory,
'jest-transform-cache-' + this._config.name,
VERSION,
);
const cacheKey = this._getCacheKey(content, filename, instrument);
const cacheKey = this._getCacheKey(
content,
filename,
instrument,
supportsDynamicImport,
supportsStaticESM,
);
// Create sub folders based on the cacheKey to avoid creating one
// directory with many files.
const cacheDir = path.join(baseCacheDir, cacheKey[0] + cacheKey[1]);
Expand Down Expand Up @@ -193,13 +203,19 @@ export default class ScriptTransformer {
return transform;
}

private _instrumentFile(filename: Config.Path, content: string): string {
private _instrumentFile(
filename: Config.Path,
content: string,
supportsDynamicImport: boolean,
supportsStaticESM: boolean,
): string {
const result = babelTransform(content, {
auxiliaryCommentBefore: ' istanbul ignore next ',
babelrc: false,
caller: {
name: '@jest/transform',
supportsStaticESM: false,
supportsDynamicImport,
supportsStaticESM,
},
configFile: false,
filename,
Expand Down Expand Up @@ -243,14 +259,23 @@ export default class ScriptTransformer {
this._getTransformer(filepath);
}

// TODO: replace third argument with TransformOptions in Jest 26
transformSource(
filepath: Config.Path,
content: string,
instrument: boolean,
supportsDynamicImport = false,
supportsStaticESM = false,
): TransformResult {
const filename = this._getRealPath(filepath);
const transform = this._getTransformer(filename);
const cacheFilePath = this._getFileCachePath(filename, content, instrument);
const cacheFilePath = this._getFileCachePath(
filename,
content,
instrument,
supportsDynamicImport,
supportsStaticESM,
);
let sourceMapPath: Config.Path | null = cacheFilePath + '.map';
// Ignore cache if `config.cache` is set (--no-cache)
let code = this._config.cache ? readCodeCacheFile(cacheFilePath) : null;
Expand Down Expand Up @@ -287,8 +312,8 @@ export default class ScriptTransformer {
if (transform && shouldCallTransform) {
const processed = transform.process(content, filename, this._config, {
instrument,
supportsDynamicImport: false,
supportsStaticESM: false,
supportsDynamicImport,
supportsStaticESM,
});

if (typeof processed === 'string') {
Expand Down Expand Up @@ -323,7 +348,12 @@ export default class ScriptTransformer {
}

if (!transformWillInstrument && instrument) {
code = this._instrumentFile(filename, transformed.code);
code = this._instrumentFile(
filename,
transformed.code,
supportsDynamicImport,
supportsStaticESM,
);
} else {
code = transformed.code;
}
Expand All @@ -350,12 +380,16 @@ export default class ScriptTransformer {

private _transformAndBuildScript(
filename: Config.Path,
options: Options | null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to have been a bug, it was never passed null 🤷‍♂

options: Options,
instrument: boolean,
fileSource?: string,
): TransformResult {
const isInternalModule = !!(options && options.isInternalModule);
const isCoreModule = !!(options && options.isCoreModule);
const {
isCoreModule,
isInternalModule,
supportsDynamicImport,
supportsStaticESM,
} = options;
const content = stripShebang(
fileSource || fs.readFileSync(filename, 'utf8'),
);
Expand All @@ -375,6 +409,8 @@ export default class ScriptTransformer {
filename,
content,
instrument,
supportsDynamicImport,
supportsStaticESM,
);

code = transformedSource.code;
Expand Down Expand Up @@ -431,8 +467,12 @@ export default class ScriptTransformer {
options: Options,
fileSource: string,
): string {
const isInternalModule = options.isInternalModule;
const isCoreModule = options.isCoreModule;
const {
isCoreModule,
isInternalModule,
supportsDynamicImport,
supportsStaticESM,
} = options;
const willTransform =
!isInternalModule && !isCoreModule && this.shouldTransform(filename);

Expand All @@ -441,6 +481,8 @@ export default class ScriptTransformer {
filename,
fileSource,
false,
supportsDynamicImport,
supportsStaticESM,
);
return transformedJsonSource;
}
Expand Down Expand Up @@ -469,7 +511,11 @@ export default class ScriptTransformer {
(code, filename) => {
try {
transforming = true;
return this.transformSource(filename, code, false).code || code;
return (
// we might wanna do `supportsDynamicImport` at some point
this.transformSource(filename, code, false, false, false).code ||
code
);
} finally {
transforming = false;
}
Expand Down