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

Jump to bottom

feat: extract 'createTransformer' and use type predicates #12407

Merged
merged 21 commits into from Apr 4, 2022
Merged

feat: extract 'createTransformer' and use type predicates #12407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
92ac49f
Remove 'createTransformer' from Transformer types
fatso83 Feb 16, 2022
036349f
Add doc strings
fatso83 Feb 16, 2022
d9b03a4
Update docs for website
fatso83 Feb 16, 2022
1c7dd1e
Export type for babel-jest
fatso83 Feb 16, 2022
c7acd5c
Make TransformerCreator generic to know if sync or async
fatso83 Feb 17, 2022
9236882
Address feedback from PR review, extracting prose when in /docs
fatso83 Feb 17, 2022
124fa09
Fix babel-jest unit tests
fatso83 Feb 17, 2022
7ccc3d7
Fix e2e transform tests
fatso83 Feb 17, 2022
b0a7b29
Make jest-repl allow transformer modules with only createTransformer
fatso83 Feb 17, 2022
561d1a4
Update tests to account for babel-jest not having default process/pro…
fatso83 Feb 17, 2022
9d310aa
PR feedback: trim down interface in /docs
fatso83 Feb 22, 2022
781c2fd
PR feedback: Add notice about brevity in trimmed down version and whe…
fatso83 Feb 22, 2022
a79850a
PR feedback: expanded explanation of how transformations are done
fatso83 Feb 22, 2022
940c43d
Apply suggestions from code review
fatso83 Feb 25, 2022
7ab0df5
Apply suggestions from code review
fatso83 Feb 28, 2022
3b04ea2
Update docs/CodeTransformation.md
fatso83 Feb 28, 2022
4df400a
prettier
SimenB Mar 1, 2022
42985ea
Merge branch 'main' into 12403-transformer-types-update
SimenB Mar 1, 2022
e5a5011
Merge branch 'main' into 12403-transformer-types-update
SimenB Apr 4, 2022
2cf267c
small cleanup
SimenB Apr 4, 2022
d62b127
changelog
SimenB Apr 4, 2022
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -75,9 +75,11 @@
- `[*]` [**BREAKING**] Drop support for `typescript@3.8`, minimum version is now `4.2` ([#11142](https://github.com/facebook/jest/pull/11142))
- `[*]` Bundle all `.d.ts` files into a single `index.d.ts` per module ([#12345](https://github.com/facebook/jest/pull/12345))
- `[*]` Use `globalThis` instead of `global` ([#12447](https://github.com/facebook/jest/pull/12447))
- `[babel-jest]` [**BREAKING**] Only export `createTransformer` ([#12407](https://github.com/facebook/jest/pull/12407))
- `[docs]` Add note about not mixing `done()` with Promises ([#11077](https://github.com/facebook/jest/pull/11077))
- `[docs, examples]` Update React examples to match with the new React guidelines for code examples ([#12217](https://github.com/facebook/jest/pull/12217))
- `[docs]` Add clarity for module factory hoisting limitations ([#12453](https://github.com/facebook/jest/pull/12453))
- `[docs]` Add more information about how code transformers work ([#12407](https://github.com/facebook/jest/pull/12407))
- `[expect]` [**BREAKING**] Remove support for importing `build/utils` ([#12323](https://github.com/facebook/jest/pull/12323))
- `[expect]` [**BREAKING**] Migrate to ESM ([#12344](https://github.com/facebook/jest/pull/12344))
- `[expect]` [**BREAKING**] Snapshot matcher types are moved to `@jest/expect` ([#12404](https://github.com/facebook/jest/pull/12404))
Expand Down
91 changes: 43 additions & 48 deletions docs/CodeTransformation.md
View file
Open in desktop
Expand Up @@ -22,72 +22,77 @@ If you override the `transform` configuration option `babel-jest` will no longer
You can write your own transformer. The API of a transformer is as follows:

```ts
// This version of the interface you are seeing on the website has been trimmed down for brevity
// For the full definition, see `packages/jest-transform/src/types.ts` in https://github.com/facebook/jest
// (taking care in choosing the right tag/commit for your version of Jest)

interface TransformOptions<OptionType = unknown> {
supportsDynamicImport: boolean;
supportsExportNamespaceFrom: boolean;
supportsStaticESM: boolean;
supportsTopLevelAwait: boolean;
instrument: boolean;
/** a cached file system which is used in jest-runtime - useful to improve performance */
cacheFS: Map<string, string>;
config: Config.ProjectConfig;
/** A stringified version of the configuration - useful in cache busting */
configString: string;
/** the options passed through Jest's config by the user */
transformerConfig: OptionType;
}

interface SyncTransformer<OptionType = unknown> {
/**
* Indicates if the transformer is capable of instrumenting the code for code coverage.
*
* If V8 coverage is _not_ active, and this is `true`, Jest will assume the code is instrumented.
* If V8 coverage is _not_ active, and this is `false`. Jest will instrument the code returned by this transformer using Babel.
*/
canInstrument?: boolean;
createTransformer?: (options?: OptionType) => SyncTransformer<OptionType>;

getCacheKey?: (
sourceText: string,
sourcePath: Config.Path,
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
sourcePath: string,
options: TransformOptions<OptionType>,
) => string;

getCacheKeyAsync?: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => Promise<string>;

process: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => TransformedSource;

processAsync?: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => Promise<TransformedSource>;
}

interface AsyncTransformer<OptionType = unknown> {
/**
* Indicates if the transformer is capable of instrumenting the code for code coverage.
*
* If V8 coverage is _not_ active, and this is `true`, Jest will assume the code is instrumented.
* If V8 coverage is _not_ active, and this is `false`. Jest will instrument the code returned by this transformer using Babel.
*/
canInstrument?: boolean;
createTransformer?: (options?: OptionType) => AsyncTransformer<OptionType>;

getCacheKey?: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => string;

getCacheKeyAsync?: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => Promise<string>;

process?: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => TransformedSource;

processAsync: (
sourceText: string,
sourcePath: Config.Path,
sourcePath: string,
options: TransformOptions<OptionType>,
) => Promise<TransformedSource>;
}
Expand All @@ -96,35 +101,25 @@ type Transformer<OptionType = unknown> =
| SyncTransformer<OptionType>
| AsyncTransformer<OptionType>;

interface TransformOptions<OptionType> {
/**
* If a transformer does module resolution and reads files, it should populate `cacheFS` so that
* Jest avoids reading the same files again, improving performance. `cacheFS` stores entries of
* <file path, file contents>
*/
cacheFS: Map<string, string>;
config: Config.ProjectConfig;
/** A stringified version of the configuration - useful in cache busting */
configString: string;
instrument: boolean;
// names are copied from babel: https://babeljs.io/docs/en/options#caller
supportsDynamicImport: boolean;
supportsExportNamespaceFrom: boolean;
supportsStaticESM: boolean;
supportsTopLevelAwait: boolean;
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
/** the options passed through Jest's config by the user */
transformerConfig: OptionType;
}

type TransformedSource =
| {code: string; map?: RawSourceMap | string | null}
| string;
type TransformerCreator<
X extends Transformer<OptionType>,
OptionType = unknown,
> = (options?: OptionType) => X;

// Config.ProjectConfig can be seen in code [here](https://github.com/facebook/jest/blob/v26.6.3/packages/jest-types/src/Config.ts#L323)
// RawSourceMap comes from [`source-map`](https://github.com/mozilla/source-map/blob/0.6.1/source-map.d.ts#L6-L12)
type TransformerFactory<X extends Transformer> = {
createTransformer: TransformerCreator<X>;
};
```

As can be seen, only `process` or `processAsync` is mandatory to implement, although we highly recommend implementing `getCacheKey` as well, so we don't waste resources transpiling the same source file when we can read its previous result from disk. You can use [`@jest/create-cache-key-function`](https://www.npmjs.com/package/@jest/create-cache-key-function) to help implement it.
There are a couple of ways you can import code into Jest - using Common JS (`require`) or ECMAScript Modules (`import` - which exists in static and dynamic versions). Jest passes files through code transformation on demand (for instance when a `require` or `import` is evaluated). This process, also known as "transpilation", might happen _synchronously_ (in the case of `require`), or _asynchronously_ (in the case of `import` or `import()`, the latter of which also works from Common JS modules). For this reason, the interface exposes both pairs of methods for asynchronous and synchronous processes: `process{Async}` and `getCacheKey{Async}`. The latter is called to figure out if we need to call `process{Async}` at all. Since async transformation can happen synchronously without issue, it's possible for the async case to "fall back" to the sync variant, but not vice versa.

So if your code base is ESM only implementing the async variants is sufficient. Otherwise, if any code is loaded through `require` (including `createRequire` from within ESM), then you need to implement the synchronous variant. Be aware that `node_modules` is not transpiled with default config.

Semi-related to this are the supports flags we pass (see `CallerTransformOptions` above), but those should be used within the transform to figure out if it should return ESM or CJS, and has no direct bearing on sync vs async

Though not required, we _highly recommend_ implementing `getCacheKey` as well, so we do not waste resources transpiling when we could have read its previous result from disk. You can use [`@jest/create-cache-key-function`](https://www.npmjs.com/package/@jest/create-cache-key-function) to help implement it.

Instead of having your custom transformer implement the `Transformer` interface directly, you can choose to export `createTransformer`, a factory function to dynamically create transformers. This is to allow having a transformer config in your jest config.

Note that [ECMAScript module](ECMAScriptModules.md) support is indicated by the passed in `supports*` options. Specifically `supportsDynamicImport: true` means the transformer can return `import()` expressions, which is supported by both ESM and CJS. If `supportsStaticESM: true` it means top level `import` statements are supported and the code will be interpreted as ESM and not CJS. See [Node's docs](https://nodejs.org/api/esm.html#esm_differences_between_es_modules_and_commonjs) for details on the differences.

Expand Down
4 changes: 2 additions & 2 deletions e2e/transform/babel-jest-async/transformer.js
View file
Open in desktop
Expand Up @@ -6,10 +6,10 @@
*/

import {fileURLToPath} from 'url';
import babelJest from 'babel-jest';
import {createTransformer} from 'babel-jest';

export default {
...babelJest.default.createTransformer({
...createTransformer({
presets: ['@babel/preset-flow'],
root: fileURLToPath(import.meta.url),
}),
Expand Down
50 changes: 18 additions & 32 deletions packages/babel-jest/src/__tests__/getCacheKey.test.ts
View file
Open in desktop
Expand Up @@ -9,6 +9,8 @@ import type {TransformOptions as BabelTransformOptions} from '@babel/core';
import type {TransformOptions as JestTransformOptions} from '@jest/transform';
import babelJest from '../index';

const {getCacheKey} = babelJest.createTransformer();
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it was either that or const babelJestTransformer = babelJest.createTransformer(); and babelJestTransformer.getCacheKey(...) (or something similar).

Since we have no other exports than createTransformer, you would otherwise need to write babelJest.createTransformer().getCacheKey(...) in every location that used babelJest.getCacheKey(...) previously.

Not sure what you prefer. Since the transformer was not used for anything besides calling its getCacheKey() method, I thought removing it and just keeping the method through destructuring was fine.

Copy link
Member

Choose a reason for hiding this comment

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

why not babelJest.getCacheKey ? It should be on the default export, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since removing all but the factory method was part of this change.

I can of course revert it, but then those exports would just be for the sake of the tests (AFAIK?); the jest-transform package will not touch any of those exports when it sees createTransformer and neither will the CLI, so I thought keeping test-only exports did not make sense from that perspective. I might be wrong, of course 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope to have addressed all non-code related feedback. Mostly used your suggested prose, apart from introducing some full stops and minor adjustments to make the sentences shorter and easier to read.

With regards to the code changes you wondered about, which mostly stems from the fact that exports that are not used outside of tests have been removed, I am not sure if you think I should revert any of those or if it made sense.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! What I'm alluding to here is that babelJest.getCacheKey should work fine, there's not need to call createTransformer first (I think!)

Copy link
Contributor Author

@fatso83 fatso83 Feb 25, 2022 •
edited

Choose a reason for hiding this comment

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

And I am saying that functionality was removed in this PR 😄 Sorry if being unclear! See https://github.com/facebook/jest/pull/12407/files#diff-5f463bc5dbe428998302335614e9bbed3b4eb3310a6075767e1b432c161d9e71L273

It used to be:

const transformer: SyncTransformer<TransformOptions> = {
  ...createTransformer(),
  createTransformer
}

export default transformer;

Note the ...createTransformer(). That is what ensures that babelJest.getCacheKey exists.

All of the fields from ...createTransformer() would be ignored by Jest itself when it sees createTransformer. Since they are not used in any actual non-test code, I thought it did not make sense to expose them, since you could always just instantiate a new transformer in the tests. Once their use is gone from tests, no code would use them. So that's the reasoning behind removing them. I could keep them, of course, which would keep the tests unchanged, but what point is there in testing exports that will never be used?


const processVersion = process.version;
const nodeEnv = process.env.NODE_ENV;
const babelEnv = process.env.BABEL_ENV;
Expand Down Expand Up @@ -39,11 +41,7 @@ describe('getCacheKey', () => {
instrument: true,
} as JestTransformOptions;

const oldCacheKey = babelJest.getCacheKey(
sourceText,
sourcePath,
transformOptions,
);
const oldCacheKey = getCacheKey(sourceText, sourcePath, transformOptions);

test('returns cache key hash', () => {
expect(oldCacheKey.length).toEqual(32);
Expand All @@ -54,9 +52,9 @@ describe('getCacheKey', () => {
readFileSync: () => 'new this file',
}));

const {default: babelJest}: typeof import('../index') = require('../index');
const {createTransformer}: typeof import('../index') = require('../index');

const newCacheKey = babelJest.getCacheKey(
const newCacheKey = createTransformer().getCacheKey(
sourceText,
sourcePath,
transformOptions,
Expand All @@ -77,9 +75,9 @@ describe('getCacheKey', () => {
};
});

const {default: babelJest}: typeof import('../index') = require('../index');
const {createTransformer}: typeof import('../index') = require('../index');

const newCacheKey = babelJest.getCacheKey(
const newCacheKey = createTransformer().getCacheKey(
sourceText,
sourcePath,
transformOptions,
Expand All @@ -89,7 +87,7 @@ describe('getCacheKey', () => {
});

test('if `sourceText` value is changing', () => {
const newCacheKey = babelJest.getCacheKey(
const newCacheKey = getCacheKey(
'new source text',
sourcePath,
transformOptions,
Expand All @@ -99,7 +97,7 @@ describe('getCacheKey', () => {
});

test('if `sourcePath` value is changing', () => {
const newCacheKey = babelJest.getCacheKey(
const newCacheKey = getCacheKey(
sourceText,
'new-source-path.js',
transformOptions,
Expand All @@ -109,7 +107,7 @@ describe('getCacheKey', () => {
});

test('if `configString` value is changing', () => {
const newCacheKey = babelJest.getCacheKey(sourceText, sourcePath, {
const newCacheKey = getCacheKey(sourceText, sourcePath, {
...transformOptions,
configString: 'new-config-string',
});
Expand All @@ -129,9 +127,9 @@ describe('getCacheKey', () => {
};
});

const {default: babelJest}: typeof import('../index') = require('../index');
const {createTransformer}: typeof import('../index') = require('../index');

const newCacheKey = babelJest.getCacheKey(
const newCacheKey = createTransformer().getCacheKey(
sourceText,
sourcePath,
transformOptions,
Expand All @@ -152,9 +150,9 @@ describe('getCacheKey', () => {
};
});

const {default: babelJest}: typeof import('../index') = require('../index');
const {createTransformer}: typeof import('../index') = require('../index');

const newCacheKey = babelJest.getCacheKey(
const newCacheKey = createTransformer().getCacheKey(
sourceText,
sourcePath,
transformOptions,
Expand All @@ -164,7 +162,7 @@ describe('getCacheKey', () => {
});

test('if `instrument` value is changing', () => {
const newCacheKey = babelJest.getCacheKey(sourceText, sourcePath, {
const newCacheKey = getCacheKey(sourceText, sourcePath, {
...transformOptions,
instrument: false,
});
Expand All @@ -175,23 +173,15 @@ describe('getCacheKey', () => {
test('if `process.env.NODE_ENV` value is changing', () => {
process.env.NODE_ENV = 'NEW_NODE_ENV';

const newCacheKey = babelJest.getCacheKey(
sourceText,
sourcePath,
transformOptions,
);
const newCacheKey = getCacheKey(sourceText, sourcePath, transformOptions);

expect(oldCacheKey).not.toEqual(newCacheKey);
});

test('if `process.env.BABEL_ENV` value is changing', () => {
process.env.BABEL_ENV = 'NEW_BABEL_ENV';

const newCacheKey = babelJest.getCacheKey(
sourceText,
sourcePath,
transformOptions,
);
const newCacheKey = getCacheKey(sourceText, sourcePath, transformOptions);

expect(oldCacheKey).not.toEqual(newCacheKey);
});
Expand All @@ -200,11 +190,7 @@ describe('getCacheKey', () => {
delete process.version;
process.version = 'new-node-version';

const newCacheKey = babelJest.getCacheKey(
sourceText,
sourcePath,
transformOptions,
);
const newCacheKey = getCacheKey(sourceText, sourcePath, transformOptions);

expect(oldCacheKey).not.toEqual(newCacheKey);
});
Expand Down