feat(transformer): async code transformation #9889

ychi · 2020-04-26T16:16:12Z

Summary

Support async code transformation as mentioned in #9504

Test plan

Test cases added

ychi · 2020-04-26T16:26:09Z

Still very early stage, but already noticing processAsync() and getCacheKeyAsync() would require multiple layers of function to be async, many of which only one or two lines of difference from the sync version. Will try to abstract the common flows into sharable functions

SimenB · 2020-04-26T17:04:57Z

Super exciting!

require multiple layers of function to be async, many of which only one or two lines of difference from the sync version

Yeah, I expected that. Almost all the logic will be identical, just cache lookups and the transforms themselves.

One thing is that multiple processes will probably want to transform the same files. Not a big deal in sync code, but for async code we'll quickly end up transpiling the same file a bunch of time, wasting a lot of CPU. DOes not have to be solved in this PR, but having some sort of mutex or lock per file name might make sense?

SimenB · 2020-04-26T17:07:08Z

packages/jest-transform/src/ScriptTransformer.ts

@@ -390,6 +491,135 @@ export default class ScriptTransformer {
    };
  }

+  // TODO: replace third argument with TransformOptions in Jest 26


we might wanna look into landing all the jest 26 changes first, not sure... Would delay this PR so probably not

ychi · 2020-04-26T18:20:18Z

One thing is that multiple processes will probably want to transform the same files. Not a big deal in sync code, but for async code we'll quickly end up transpiling the same file a bunch of time, wasting a lot of CPU. DOes not have to be solved in this PR, but having some sort of mutex or lock per file name might make sense?

Yes that makes sense. A way to implement per file name lock might be shared module map that tracks file name, whether being transpiled, and cached transpile result?

SimenB · 2020-04-26T18:40:30Z

Yeah, something like that 🙂 Again, not something we need in a first iteration, but something to keep in mind

ychi · 2020-04-27T08:21:25Z

Considering the following case: a file is associated with a processAsync-only transformer in the cache, but a synchronous transform on it is attempted. We can only throw an error in this case?

If code cache is shared between processes in the future, might be able to reduce this kind of error? (When the file was transformed asynchronously and cached before the sync call)

SimenB · 2020-04-27T08:31:07Z

We can only throw an error in this case?

Yes.

If code cache is shared between processes in the future, might be able to reduce this kind of error?

I don't think we should do that, it would make tests passing dependent on execution order (or warm cache), which we don't want

ychi · 2020-05-03T15:09:22Z

Hi @SimenB, can you take a look and see it the changes are on the right direction? Also, a bit curious why tsc complaints about require('../package.json'); which I hadn't touched 🤔

SimenB · 2020-05-03T15:11:28Z

Try a rebase, I've had weird warnings like that before that a rebase took care of

SimenB

Currently on mobile, so a bit painful to review. Skimmed over and I like how this is shaping up! 👍

SimenB · 2020-05-03T15:19:30Z

packages/jest-transform/src/ScriptTransformer.ts

+        return transformer;
+      }
+
+      transform = await import(transformPath);


If this is to import ESM we need to add this gile to the blacklist in babel.config.js in the root of the repo.

packages/jest-transform/src/types.ts

+    options: CacheKeyOptions,
+  ) => Promise<string>;
+
+  process: (


SimenB · 2020-05-03T15:22:33Z

packages/jest-transform/src/types.ts

+    options?: TransformOptions,
+  ) => TransformedSource;
+
+  processAsync?: (


Not optional

ychi · 2020-05-10T20:15:55Z

Regarding tsc package.json is not under 'rootDir' complaint, it happens when import() is also used in the file. So it's not issue with git. Haven't found much discussion about this. Will play around with it more and maybe reach out to TypeScript community.

Update: there is include property available in tsconfig, using it instead of rootDir seems to work.

codecov-io · 2020-05-10T21:12:11Z

Codecov Report

Merging #9889 (fc92ed8) into master (f73d5f6) will increase coverage by 0.16%.
The diff coverage is 98.88%.

@@            Coverage Diff             @@
##           master    #9889      +/-   ##
==========================================
+ Coverage   64.14%   64.31%   +0.16%     
==========================================
  Files         307      307              
  Lines       13375    13444      +69     
  Branches     3260     3281      +21     
==========================================
+ Hits         8580     8647      +67     
- Misses       4089     4090       +1     
- Partials      706      707       +1

Impacted Files	Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts	`72.00% <98.87%> (+8.36%)`	⬆️
packages/babel-jest/src/index.ts	`54.05% <100.00%> (ø)`
packages/expect/src/utils.ts	`94.83% <0.00%> (-1.30%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f73d5f6...fc92ed8. Read the comment docs.

ychi · 2020-05-18T19:46:12Z

Hi @SimenB:
What was the motivation to use rootDir over include in tsconfig.json? I think there is a bug with tsc and has reported it. By using include, we won't bump into that issue. The following packages also require package.json so might encounter similar situation somewhere down the road:

jest-core
jest-haste-map
jest-runtime
Does it make sense to switch to include instead?

SimenB · 2020-05-19T16:33:50Z

No particular reason. If it works I'm happy to merge a PR migrating

ychi · 2020-05-21T15:19:37Z

I was wrong. When we use include instead of rootDir, tsc does not complain about "outside of rootDir" but does not generate proper output for the package either :(

SimenB

finally 😅

I think this looks really good, thanks for sticking with it @ychi! Beyond the nits I've left, I wonder (as mentioned) if we should look at solving #11081 at the same time. We can probably punt on it since it'd just streamline loading transformers and is not related to the async transformation itself. done in #11163

@ychi can you add a changelog entry and merge in (or rebase, whatever you prefer) master?

SimenB · 2021-03-05T07:45:35Z

packages/jest-transform/src/ScriptTransformer.ts

+      content,
+      options,
+    );
+    const sourceMapPath: Config.Path | null = cacheFilePath + '.map';


this cannot be null - it's a const and you assign a concatenated string

null + '.map' "null.map"

The current code sets it to null if we don't have a source map - that seems sensible. I think that condition is missing here

True, that setting to null statement now happens in _buildTransformResult. Removed the null type here (and the one in transformSource)

SimenB · 2021-03-05T07:49:29Z

packages/jest-transform/src/ScriptTransformer.ts

@@ -26,14 +26,20 @@ import {
 } from 'jest-util';
 import handlePotentialSyntaxError from './enhanceUnexpectedTokenMessage';
 import shouldInstrument from './shouldInstrument';
+// eslint somehow complains "There should be no empty line between import groups"


also if you remove the new newline on line 42?

Removing that new newline works, thank you!

SimenB · 2021-03-05T07:51:44Z

packages/jest-transform/src/ScriptTransformer.ts

+      this._getTransformer(filename) || {};
+    let transformerCacheKey = undefined;
+
+    if (transformer && typeof transformer.getCacheKey === 'function') {


Suggested change

if (transformer && typeof transformer.getCacheKey === 'function') {

if (typeof transformer?.getCacheKey === 'function') {

SimenB · 2021-03-05T07:54:13Z

packages/jest-transform/src/ScriptTransformer.ts

+          transformerConfig,
+        },
+      );
+    } else if (transformer && typeof transformer.getCacheKey === 'function') {


Suggested change

} else if (transformer && typeof transformer.getCacheKey === 'function') {

} else if (typeof transformer?.getCacheKey === 'function') {

Actually, removed the duplication and using await getCacheKey now

SimenB · 2021-03-05T07:54:41Z

packages/jest-transform/src/ScriptTransformer.ts

+      (await this._getTransformerAsync(filename)) || {};
+    let transformerCacheKey = undefined;
+
+    if (transformer && typeof transformer.getCacheKeyAsync === 'function') {


Suggested change

if (transformer && typeof transformer.getCacheKeyAsync === 'function') {

if (typeof transformer?.getCacheKeyAsync === 'function') {

SimenB · 2021-03-05T08:02:18Z

packages/jest-transform/src/ScriptTransformer.ts

+      return cached;
+    }
+
+    let transformer: AsyncTransformer = await import(transformPath);


we should have a mutex here. Just a Map<Config.Path, Promise<void>> where the Path is transformPath and we can await that above the this._transformCache.get call. If we get a cache miss, we add a promise to the map there. that way the cache will be populated once.

checking if the map has a promise before awaiting will avoid the extra tick as well.

(make sure the promise we stick in the cache resolve to something empty so we don't keep a reference to something we don't need)

Modified, the Promise only gets resolved after cache has been set. A bit curious whether we can make cache and the mutex single source of truth while allowing transformer sync loading to work as-is 🤔

packages/jest-transform/src/ScriptTransformer.ts

SimenB · 2021-03-05T08:09:25Z

packages/jest-transform/src/ScriptTransformer.ts

@@ -191,7 +313,7 @@ export default class ScriptTransformer {
      return cached;
    }

-    let transformer: Transformer = require(transformPath);
+    let transformer: SyncTransformer = require(transformPath);


we might need to refactor to load all transformers in some warmup step later so people can write their transformers in ESM (which cannot be required). Not sure if we should stick the refactor in here as well... Thoughts?

(To be clear, the transformer itself can be written in ESM but still expose sync transformations. Concrete example: #11081. While that talks about doing the import() in an async thing before using and the transformer itself being CJS, I think people will expect to be able to write ESM directly)

I've implemented this in #11163

SimenB · 2021-03-05T08:10:01Z

packages/jest-transform/src/ScriptTransformer.ts

      if (typeof processed === 'string') {
        transformed.code = processed;
      } else if (processed != null && typeof processed.code === 'string') {
        transformed = processed;
      } else {
        throw new TypeError(
          "Jest: a transform's `process` function must return a string, " +
-            'or an object with `code` key containing this string.',
+            'or an object with `code` key containing this string. ' +
+            "It's `processAsync` function must return that in a promise.",


Suggested change

"It's `processAsync` function must return that in a promise.",

"It's `processAsync` function must return a Promise resolving to it.",

SimenB · 2021-03-05T08:10:39Z

packages/jest-transform/src/ScriptTransformer.ts

@@ -371,6 +470,9 @@ export default class ScriptTransformer {
    if (map) {
      const sourceMapContent =
        typeof map === 'string' ? map : JSON.stringify(map);
+
+      invariant(sourceMapPath, 'We should always have default sourceMapPath');


I don't think this can ever not be set

I add this because sourceMapPath might be set to null, making the type Config.Path | null in this function.

Another approach might be declaring a new local variable for return?

SimenB · 2021-03-06T15:31:02Z

@ychi FYI I've opened up #11163 which will conflict horribly with this PR

ychi · 2021-03-06T23:24:23Z

Let me try to think through the mutex part this weekend and see if I can get ahead of the race😂

ychi · 2021-03-07T12:06:57Z

@SimenB: feedback addressed and added changelog.
MacOS test run on Azure somehow failed 😬

SimenB · 2021-03-07T11:30:14Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 ### Features

+-`[jest-transform]` Async code transformations ([#9889](https://github.com/facebook/jest/pull/9889))


Should be sorted alphabetically

SimenB · 2021-03-07T12:39:27Z

packages/jest-transform/src/ScriptTransformer.ts

+      return null;
+    }
+
+    if (this._transformCacheMutex.has(transformPath)) {


The changes in #11163 will make sure all transformers are loaded before starting any transformation at all. So we don't actually need this anymore 😅

I wonder if we should have a mutex on the transformation itself, though? I.e. on the cache key... I'll have a play with that when I test this PR together with the changes in jest-runtime to make use of it

Just remembered we briefly touched on this idea 😀

Hah, there you go! I agree with past me - both in that we should explore it and that we don't necessarily have to do anything about it before landing this PR

SimenB · 2021-03-07T14:42:41Z

OK, landed the other one. Sorry about the conflicts 🙈 I think the changes over there makes the changes in this PR a bit cleaner though, as it centralizes "load transformer", and for async transformation we only need to deal with async cache key and async transformation.

Tell me if you want help resolving the conflicts

ychi · 2021-03-07T18:33:54Z

@SimenB Some help would be wonderful😆 I am relocating in the coming week, available time might be fragmented. If you (or anyone) wants to chime in, please feel free. Otherwise I can come back to this in mid- to late-March. 🙂

SimenB · 2021-03-07T20:11:09Z

Squashed, removed the mutex, and rebased except for the very latest commit. That one is a bit more hairy since we have a bunch of tests which assert we throw error about missing process during transform while that has been moved to load now. So I just pushed what I'm certain about now. Will do the rest as well

SimenB · 2021-03-07T21:04:07Z

@ychi I think this came out correctly. The tests were a bit hairy, but I think I got it 😅

netlify · 2021-03-13T12:55:16Z

Deploy preview for jestjs ready!

Built without sensitive environment variables with commit fc92ed8

https://deploy-preview-9889--jestjs.netlify.app

SimenB · 2021-03-13T14:21:37Z

I've played with using this in the runtime, and I think we should be good to go 👍

Need to add some docs, but I can do that now.

EDIT: actually, docs should come with the change in runtime, so I'll land this now. Thanks for you patience and perseverance @ychi!

github-actions · 2021-05-10T21:07:13Z

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 Apr 26, 2020

ychi changed the title l [Work In Progress] Async Code Transformation Apr 26, 2020

ychi marked this pull request as draft April 26, 2020 16:30

SimenB reviewed Apr 26, 2020

View reviewed changes

ychi force-pushed the async-transform branch 2 times, most recently from ca916bf to f85e570 Compare May 3, 2020 09:57

ychi requested a review from SimenB May 3, 2020 15:09

SimenB reviewed May 3, 2020

View reviewed changes

ychi force-pushed the async-transform branch 2 times, most recently from 3ea5f17 to d4f36fa Compare May 10, 2020 17:03

ychi force-pushed the async-transform branch from d4f36fa to 05e632d Compare May 10, 2020 20:45

mihar-22 mentioned this pull request May 18, 2020

Preprocessor errors are hidden svelteness/svelte-jester#5

Closed

ychi force-pushed the async-transform branch from 05e632d to 49aac1d Compare May 19, 2020 20:53

ychi force-pushed the async-transform branch from 49aac1d to b53603e Compare June 1, 2020 10:56

ychi force-pushed the async-transform branch from b53603e to 0919547 Compare June 27, 2020 17:42

ychi force-pushed the async-transform branch from c5d3537 to 2537d4a Compare July 14, 2020 19:02

ychi force-pushed the async-transform branch from 2537d4a to 3a578d9 Compare July 26, 2020 14:54

SimenB added this to the Jest 27 milestone Mar 4, 2021

SimenB approved these changes Mar 5, 2021

View reviewed changes

SimenB mentioned this pull request Mar 5, 2021

Implement async code transformations #9504

Closed

ychi force-pushed the async-transform branch 2 times, most recently from cd91c8d to 141b722 Compare March 7, 2021 10:30

ychi requested a review from SimenB March 7, 2021 12:07

SimenB reviewed Mar 7, 2021

View reviewed changes

SimenB force-pushed the async-transform branch from 3c1e5ca to 2d2f5e6 Compare March 7, 2021 20:09

SimenB force-pushed the async-transform branch 2 times, most recently from 6a62fb2 to 7707816 Compare March 7, 2021 21:03

SimenB requested review from thymikee and jeysal March 7, 2021 21:08

Adding transformAsync

fc92ed8

SimenB force-pushed the async-transform branch from 7707816 to fc92ed8 Compare March 13, 2021 12:50

SimenB changed the title ~~Async Code Transformation~~ feat(transformer): async code transformation Mar 13, 2021

SimenB merged commit 858c50b into jestjs:master Mar 13, 2021

SimenB mentioned this pull request Mar 13, 2021

feat(runtime): add support for async code transformation #11191

Merged

SimenB mentioned this pull request Mar 30, 2021

Meta: Native support for ES Modules #9430

Open

21 tasks

SimenB mentioned this pull request May 1, 2021

Implement async dependency resolution #9505

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021

	if (transformer && typeof transformer.getCacheKey === 'function') {
	if (typeof transformer?.getCacheKey === 'function') {

	} else if (transformer && typeof transformer.getCacheKey === 'function') {
	} else if (typeof transformer?.getCacheKey === 'function') {

	if (transformer && typeof transformer.getCacheKeyAsync === 'function') {
	if (typeof transformer?.getCacheKeyAsync === 'function') {

	"It's `processAsync` function must return that in a promise.",
	"It's `processAsync` function must return a Promise resolving to it.",

		@@ -2,6 +2,8 @@

		### Features

		-`[jest-transform]` Async code transformations ([#9889](https://github.com/facebook/jest/pull/9889))

feat(transformer): async code transformation #9889

feat(transformer): async code transformation #9889

Conversation

ychi commented Apr 26, 2020 • edited Loading

Summary

Test plan

ychi commented Apr 26, 2020

SimenB commented Apr 26, 2020

Choose a reason for hiding this comment

ychi commented Apr 26, 2020

SimenB commented Apr 26, 2020

ychi commented Apr 27, 2020

SimenB commented Apr 27, 2020

ychi commented May 3, 2020

SimenB commented May 3, 2020

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

ychi commented May 10, 2020 • edited Loading

codecov-io commented May 10, 2020 • edited Loading

Codecov Report

ychi commented May 18, 2020

SimenB commented May 19, 2020

ychi commented May 21, 2020

SimenB left a comment • 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

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

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

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 commented Mar 6, 2021

ychi commented Mar 6, 2021

ychi commented Mar 7, 2021

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 commented Mar 7, 2021 • edited Loading

ychi commented Mar 7, 2021

SimenB commented Mar 7, 2021

SimenB commented Mar 7, 2021

netlify bot commented Mar 13, 2021

SimenB commented Mar 13, 2021 • edited Loading

github-actions bot commented May 10, 2021

ychi commented Apr 26, 2020 •

edited

Loading

ychi commented May 10, 2020 •

edited

Loading

codecov-io commented May 10, 2020 •

edited

Loading

SimenB left a comment •

edited

Loading

SimenB commented Mar 7, 2021 •

edited

Loading

SimenB commented Mar 13, 2021 •

edited

Loading