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

Implement async code transformations #9504

Closed
SimenB opened this issue Feb 3, 2020 · 22 comments · Fixed by #11191
Closed

Implement async code transformations #9504

SimenB opened this issue Feb 3, 2020 · 22 comments · Fixed by #11191

Comments

@SimenB
Copy link
Member

SimenB commented Feb 3, 2020

EcmaScript Modules can (actually must, but easy to wrap the sync stuff in a promise) resolve asynchronously. While it's in no way a blocker for ESM support in Jest, it would be nice to add support for async transforms to be used.

This logic should for now not impact anything in Jest beyond @jest/transform as Jest wouldn't use it until ESM support is in place.

I'm thinking we can add a .transformAsync and .transformSourceAsync to the ScriptTransformer class.

There are some checks and verifications that process is a function - we need to make sure there is one clear code path for sync and one for async transformation that verify the provided reporter implements the correct interface.

Sort of reopening #5556 and all its linked issues.

@the-spyke
Copy link
Contributor

@SimenB I was looking into babel-jest because it can't load ESM config files: Babel demands to load them asynchronously. And looks like not only Transformer.process() needs to be async, but Transformer.getCacheKey() too.

@SimenB
Copy link
Member Author

SimenB commented Feb 26, 2020

Ah, to load Babel's config? That makes sense.

@ahnpnl
Copy link
Contributor

ahnpnl commented Mar 22, 2020

Does this affect to current custom transformers’ implementation ? I’m curious about the performance can be gained with async

@the-spyke
Copy link
Contributor

ScriptTransformer also should load transformers with import() to support ESM transformers (just bumped into this).

@SimenB
Copy link
Member Author

SimenB commented Apr 4, 2020

@ahnpnl not unless they want to, it'll be opt-in. If the transformer doesn't export async functions, we'll fallback to the synchronous ones.

@the-spyke good point! I'm starting to wonder if we should export both SyncScriptTransformer (the one we have today) and AsyncScriptTransformer. I started work on this in #9597, and it already feels a bit hairy trying to juggle sync vs async. Might be cleaner with two separate classes (who could both extend from some base class with the base of implementation, or import shared helpers)

@ahnpnl
Copy link
Contributor

ahnpnl commented Apr 10, 2020

I did some experiment yesterday with jest-worker, end up I noticed I had to wait for this. This can help a lot for ts-jest to separate type checking and compiling.

@ychi
Copy link
Contributor

ychi commented Apr 19, 2020

I am interested in this, can I help?

@SimenB
Copy link
Member Author

SimenB commented Apr 20, 2020

Yes please! It's somewhat invasive, but I'll be happy to work on it with you.

I haven't figured out if it makes the most sense to add transformAsync and tranformSourceAsync to ScriptTransformer class, or if an AsyncScriptTransformer class should be created. Probably the former?

Regardless, these functions should then call transformer.processAsync and transformer.getCacheKeyAsync if they exist, falling back to the synchronous methods.

Let's start with just adding support to @jest/transform somehow, then separately tackle calling this new API from the ESM parts of jest-runtime and adding support to babel-jest

@ychi
Copy link
Contributor

ychi commented Apr 21, 2020

Thank you, @SimenB!

Just so I understand it correctly:
There are 3 major steps of ES module instance graph building: construction, instantiation and evaluation, and each step can be asynchronous. Transform happens during construction phase, after fetching the file; while parsing the file, it will be transformed/transpiled.

We want to enable transformation itself to be async, that means when client calls ScriptTransformer.transformAsync() or ScriptTransformer.transformSourceAsync(), promises will be returned. If processAsync and getCacheKeyAsync of Transformer are not in place, resolved promise will be returned.

No matter we are going transformAsync or AsyncScriptTransformer, I think we will need an AsyncTransformer that extends Transformer interface? 🙂

@SimenB
Copy link
Member Author

SimenB commented Apr 21, 2020

Just so I understand it correctly:

yup, that summary sounds correct.

No matter we are going transformAsync or AsyncScriptTransformer, I think we will need an AsyncTransformer that extends Transformer interface? 🙂

I'm not sure if we should force transformers to work in sync mode - there might be cases where a transformer won't work synchronously, and we shouldn't ban that use case.

So I'm thinking 2 separate interfaces, one with optional async methods, and one with optional sync methods.

@ahnpnl
Copy link
Contributor

ahnpnl commented Apr 22, 2020

maybe my question here won't be related to this feature but is it possible to have a post process hook in ScriptTransformer which is called once all the files have been transformed ?

@ychi
Copy link
Contributor

ychi commented Apr 23, 2020

So I'm thinking 2 separate interfaces, one with optional async methods, and one with optional sync methods.

Do you mean something like the following?

interface STransformer {
  ...
  process: (
    ...
  ) => TransformedSource;
  processAsync?: (
    ...
  ) => Promise<TransformedSource>;
}

interface ATransformer {
  ...
  process?: (
    ...
  ) => TransformedSource;
  processAsync: (
    ...
  ) => Promise<TransformedSource>;
}

And transformAsync() always check if processAsync is there, if not, it will use process and wrap the result in a promise. But it looks like STransformer will just do the job?

maybe my question here won't be related to this feature but is it possible to have a post process hook in ScriptTransformer which is called once all the files have been transformed ?

transform() only handle one file at a time, maybe there is a client of ScriptTransformer that calls ScriptTransformer.transform() for each file?

@SimenB
Copy link
Member Author

SimenB commented Apr 23, 2020

maybe my question here won't be related to this feature but is it possible to have a post process hook in ScriptTransformer which is called once all the files have been transformed ?

There is no point at which "all files have been transformed" as there might be import() or require() calls all over the place, even as the last thing that happens in a test file.


Do you mean something like the following?

Yup!

@ahnpnl
Copy link
Contributor

ahnpnl commented Apr 23, 2020

Is there a way for transformer to access some sort of “after test run finish” hook ? I know there are some hooks which are available for watch plugin but transformer doesn’t have.

@SimenB
Copy link
Member Author

SimenB commented Apr 23, 2020

Not really at the moment. You could implement it as a reporter, but I'm guessing you want to access some state or some such... Transformers can run in different workers though, so even that might not work.

Regardless, not really related to this issue. Could you open up a new one, and describe your use case for it?

@nicolo-ribaudo
Copy link
Contributor

(Partially off topic) I'm exploring shipping Babel as native ESM, and this means that it would be impossible to synchronously do const babel = require("@babel/core"); babel.transformSync(...).

Does Jest have a mechanism to pre-load things before compiling (const babel = await import("@babel/core")), or would it be unable to update to Babel 8 until async transforms are supported?

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2021

That would break current transformers indeed. We can add some sort of "preload" to transformers where they can do whatever they want async so that they're ready to transform code synchronously (e.g. do import('@babel/core')) like you suggest. We need to keep supporting synchronous transformation for JIT when people use require, but some sort of bootstrapping phase should be fine.

Can you open up a new issue with that feature request? SOrta orthogonal to async transformation

@ahnpnl
Copy link
Contributor

ahnpnl commented Feb 12, 2021

I like the idea of having "preload" phase for transformers. That will solve quite some problems with performance for ts-jest. Also any information from "preload" should be passed to transformer instance.

Currently I am experimenting a workaround is

// jest.config.js
const preloadData = require('my-preload-script')

module.exports = {
    transform:{
        '^.+\\.tsx?$': ['ts-jest', { preloadData }] 
    }
}

so in this workaround, I can pass preloadData to all transformer instances in all workers. Not sure if it's the recommend way.

@SimenB
Copy link
Member Author

SimenB commented Feb 12, 2021

Let's discuss this in a separate issue 🙂

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2021

Gave this some more though when working on #11150 (leaving in an extra async that isn't needed, but will be when this issue is fixed). And I think it'll be quite straightforward to implement in jest-runtime once #9889 lands. I'll do it soonish, definitely before a stable release of Jest 27 even though it shouldn't be a breaking change. That way people with async transforms have a major release they can target

@SimenB
Copy link
Member Author

SimenB commented Mar 13, 2021

#11191

@github-actions
Copy link

This issue 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants