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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(transform): refactor API to pass an options bag around rather than multiple boolean options #10753

Merged
merged 1 commit into from Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,7 @@
### Fixes

- `[jest-transform]` Show enhanced `SyntaxError` message for all `SyntaxError`s ([#10749](https://github.com/facebook/jest/pull/10749))
- `[jest-transform]` [**BREAKING**] Refactor API to pass an options bag around rather than multiple boolean options ([#10753](https://github.com/facebook/jest/pull/10753))

### Chore & Maintenance

Expand Down
2 changes: 1 addition & 1 deletion packages/jest-reporters/src/generateEmptyCoverage.ts
Expand Up @@ -70,7 +70,7 @@ export default function (
const {code} = new ScriptTransformer(config).transformSource(
filename,
source,
true,
{instrument: true},
);
// TODO: consider passing AST
const extracted = readInitialCoverage(code);
Expand Down
108 changes: 36 additions & 72 deletions packages/jest-transform/src/ScriptTransformer.ts
Expand Up @@ -28,6 +28,7 @@ import handlePotentialSyntaxError from './enhanceUnexpectedTokenMessage';
import shouldInstrument from './shouldInstrument';
import type {
Options,
TransformOptions,
TransformResult,
TransformedSource,
Transformer,
Expand Down Expand Up @@ -92,9 +93,7 @@ export default class ScriptTransformer {
private _getCacheKey(
fileData: string,
filename: Config.Path,
instrument: boolean,
supportsDynamicImport: boolean,
supportsStaticESM: boolean,
options: TransformOptions,
): string {
const configString = this._cache.configString;
const transformer = this._getTransformer(filename);
Expand All @@ -104,10 +103,12 @@ export default class ScriptTransformer {
.update(
transformer.getCacheKey(fileData, filename, configString, {
config: this._config,
instrument,
instrument: options.instrument,
rootDir: this._config.rootDir,
supportsDynamicImport,
supportsStaticESM,
supportsDynamicImport: options.supportsDynamicImport,
supportsExportNamespaceFrom: options.supportsExportNamespaceFrom,
supportsStaticESM: options.supportsStaticESM,
supportsTopLevelAwait: options.supportsTopLevelAwait,
}),
)
.update(CACHE_VERSION)
Expand All @@ -116,7 +117,7 @@ export default class ScriptTransformer {
return createHash('md5')
.update(fileData)
.update(configString)
.update(instrument ? 'instrument' : '')
.update(options.instrument ? 'instrument' : '')
.update(filename)
.update(CACHE_VERSION)
.digest('hex');
Expand All @@ -126,22 +127,14 @@ export default class ScriptTransformer {
private _getFileCachePath(
filename: Config.Path,
content: string,
instrument: boolean,
supportsDynamicImport: boolean,
supportsStaticESM: boolean,
options: TransformOptions,
): Config.Path {
const baseCacheDir = HasteMap.getCacheFilePath(
this._config.cacheDirectory,
'jest-transform-cache-' + this._config.name,
VERSION,
);
const cacheKey = this._getCacheKey(
content,
filename,
instrument,
supportsDynamicImport,
supportsStaticESM,
);
const cacheKey = this._getCacheKey(content, filename, options);
// 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 @@ -212,9 +205,8 @@ export default class ScriptTransformer {
private _instrumentFile(
filename: Config.Path,
input: TransformedSource,
supportsDynamicImport: boolean,
supportsStaticESM: boolean,
canMapToInput: boolean,
options: TransformOptions,
): TransformedSource {
const inputCode = typeof input === 'string' ? input : input.code;
const inputMap = typeof input === 'string' ? null : input.map;
Expand All @@ -224,8 +216,10 @@ export default class ScriptTransformer {
babelrc: false,
caller: {
name: '@jest/transform',
supportsDynamicImport,
supportsStaticESM,
supportsDynamicImport: options.supportsDynamicImport,
supportsExportNamespaceFrom: options.supportsExportNamespaceFrom,
supportsStaticESM: options.supportsStaticESM,
supportsTopLevelAwait: options.supportsTopLevelAwait,
},
configFile: false,
filename,
Expand Down Expand Up @@ -259,23 +253,14 @@ export default class ScriptTransformer {
this._getTransformer(filepath);
}

// TODO: replace third argument with TransformOptions in Jest 26
Copy link
Member Author

Choose a reason for hiding this comment

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

😅

transformSource(
filepath: Config.Path,
content: string,
instrument: boolean,
supportsDynamicImport = false,
supportsStaticESM = false,
options: TransformOptions,
): TransformResult {
const filename = tryRealpath(filepath);
const transform = this._getTransformer(filename);
const cacheFilePath = this._getFileCachePath(
filename,
content,
instrument,
supportsDynamicImport,
supportsStaticESM,
);
const cacheFilePath = this._getFileCachePath(filename, content, options);
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 @@ -305,11 +290,12 @@ export default class ScriptTransformer {
};

if (transform && shouldCallTransform) {
const processed = transform.process(content, filename, this._config, {
instrument,
supportsDynamicImport,
supportsStaticESM,
});
const processed = transform.process(
content,
filename,
this._config,
options,
);

if (typeof processed === 'string') {
transformed.code = processed;
Expand Down Expand Up @@ -343,7 +329,7 @@ export default class ScriptTransformer {

// Apply instrumentation to the code if necessary, keeping the instrumented code and new map
let map = transformed.map;
if (!transformWillInstrument && instrument) {
if (!transformWillInstrument && options.instrument) {
/**
* We can map the original source code to the instrumented code ONLY if
* - the process of transforming the code produced a source map e.g. ts-jest
Expand All @@ -359,9 +345,8 @@ export default class ScriptTransformer {
const instrumented = this._instrumentFile(
filename,
transformed,
supportsDynamicImport,
supportsStaticESM,
shouldEmitSourceMaps,
options,
);

code =
Expand Down Expand Up @@ -391,15 +376,10 @@ export default class ScriptTransformer {
private _transformAndBuildScript(
filename: Config.Path,
options: Options,
instrument: boolean,
transformOptions: TransformOptions,
fileSource?: string,
): TransformResult {
const {
isCoreModule,
isInternalModule,
supportsDynamicImport,
supportsStaticESM,
} = options;
const {isCoreModule, isInternalModule} = options;
const content = stripShebang(
fileSource || fs.readFileSync(filename, 'utf8'),
Copy link
Member Author

@SimenB SimenB Oct 31, 2020

Choose a reason for hiding this comment

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

jest-runtime already sends in the source, seemed like a good time to remove this fallback

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh, too painful to fix the tests, can circle back to it later. I pushed WIP stuff here: SimenB@31d67e2

);
Expand All @@ -410,16 +390,14 @@ export default class ScriptTransformer {
const willTransform =
!isInternalModule &&
!isCoreModule &&
(this.shouldTransform(filename) || instrument);
(transformOptions.instrument || this.shouldTransform(filename));

try {
if (willTransform) {
const transformedSource = this.transformSource(
filename,
content,
instrument,
supportsDynamicImport,
supportsStaticESM,
transformOptions,
);

code = transformedSource.code;
Expand Down Expand Up @@ -458,7 +436,7 @@ export default class ScriptTransformer {
const result = this._transformAndBuildScript(
filename,
options,
instrument,
{...options, instrument},
fileSource,
);

Expand All @@ -474,22 +452,15 @@ export default class ScriptTransformer {
options: Options,
fileSource: string,
): string {
const {
isCoreModule,
isInternalModule,
supportsDynamicImport,
supportsStaticESM,
} = options;
const {isCoreModule, isInternalModule} = options;
const willTransform =
!isInternalModule && !isCoreModule && this.shouldTransform(filename);

if (willTransform) {
const {code: transformedJsonSource} = this.transformSource(
filename,
fileSource,
false,
supportsDynamicImport,
supportsStaticESM,
{...options, instrument: false},
);
return transformedJsonSource;
}
Expand All @@ -500,14 +471,17 @@ export default class ScriptTransformer {
requireAndTranspileModule<ModuleType = unknown>(
moduleName: string,
callback?: (module: ModuleType) => void,
transformOptions?: TransformOptions,
): ModuleType;
requireAndTranspileModule<ModuleType = unknown>(
moduleName: string,
callback?: (module: ModuleType) => Promise<void>,
transformOptions?: TransformOptions,
): Promise<ModuleType>;
requireAndTranspileModule<ModuleType = unknown>(
moduleName: string,
callback?: (module: ModuleType) => void | Promise<void>,
transformOptions: TransformOptions = {instrument: false},
): ModuleType | Promise<ModuleType> {
// Load the transformer to avoid a cycle where we need to load a
// transformer in order to transform it in the require hooks
Expand All @@ -519,9 +493,7 @@ export default class ScriptTransformer {
try {
transforming = true;
return (
// we might wanna do `supportsDynamicImport` at some point
Copy link
Member Author

Choose a reason for hiding this comment

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

allowing consumers of this API to decide seems best

this.transformSource(filename, code, false, false, false).code ||
code
this.transformSource(filename, code, transformOptions).code || code
);
} finally {
transforming = false;
Expand Down Expand Up @@ -562,14 +534,6 @@ export default class ScriptTransformer {
return module;
}

/**
* @deprecated use `this.shouldTransform` instead
*/
// @ts-expect-error: Unused and private - remove in Jest 25
private _shouldTransform(filename: Config.Path): boolean {
return this.shouldTransform(filename);
}

shouldTransform(filename: Config.Path): boolean {
const ignoreRegexp = this._cache.ignorePatternsRegExp;
const isIgnored = ignoreRegexp ? ignoreRegexp.test(filename) : false;
Expand Down
Expand Up @@ -71,8 +71,10 @@ Object {
},
"instrument": true,
"rootDir": "/",
"supportsDynamicImport": false,
"supportsStaticESM": false,
"supportsDynamicImport": undefined,
"supportsExportNamespaceFrom": undefined,
"supportsStaticESM": undefined,
"supportsTopLevelAwait": undefined,
}
`;

Expand Down
Expand Up @@ -338,7 +338,7 @@ describe('ScriptTransformer', () => {
};
const scriptTransformer = new ScriptTransformer(config);
expect(() =>
scriptTransformer.transformSource('sample.js', '', false),
scriptTransformer.transformSource('sample.js', '', {instrument: false}),
).toThrow('Jest: a transform must export a `process` function.');
});

Expand All @@ -355,7 +355,7 @@ describe('ScriptTransformer', () => {
};
const scriptTransformer = new ScriptTransformer(config);
expect(() =>
scriptTransformer.transformSource('sample.js', '', false),
scriptTransformer.transformSource('sample.js', '', {instrument: false}),
).toThrow('Jest: a transform must export a `process` function.');
});

Expand All @@ -366,7 +366,7 @@ describe('ScriptTransformer', () => {
};
const scriptTransformer = new ScriptTransformer(config);
expect(() =>
scriptTransformer.transformSource('sample.js', '', false),
scriptTransformer.transformSource('sample.js', '', {instrument: false}),
).not.toThrow();
});

Expand Down