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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support an async "preload" phase for source transformers #11081

Closed
nicolo-ribaudo opened this issue Feb 12, 2021 · 18 comments
Closed

Support an async "preload" phase for source transformers #11081

nicolo-ribaudo opened this issue Feb 12, 2021 · 18 comments

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Feb 12, 2021

馃殌 Feature Proposal

Source transformers (such as babel-jest) should be allowed to have a one-time asynchronous initialization step, to load external resources or dependencies.

This step must be executed before compiling, so that require() calls can still be intercepted and handled synchronously.

Motivation

We are exploring publishing Babel as ESM starting from Babel 8. This means that would not be possible to synchronously require("@babel/core") anymore, but babel-jest would need to load it with await import("@babel/core").

Note that if Babel will be the only reason to implement this feature, we should wait until we are 100% sure that Babel will be released as ESM.

Example

I don't know how the source transformers API works 馃槄 If anyone can suggest an example, I can edit this description

Pitch

Why does this feature belong in the Jest core platform?

Transformers are already part of the core platform, and it's impossible to implement it externally.

Ref #9504 (comment), cc @ahnpnl

@SimenB
Copy link
Member

SimenB commented Feb 12, 2021

Immediate idea is just to add optional async preload() the Transformer: https://github.com/facebook/jest/blob/42f78d49231dd7828f862f36c916c30346487768/packages/jest-transform/src/types.ts#L66-L81

However, @ahnpnl said in #9504 (comment)

Also any information from "preload" should be passed to transformer instance.

I don't really like that. Maybe we should go with a class instead of a plain object? Then that class can stick whatever it wants as a property on its instances.

Thoughts?

/cc @jeysal @thymikee


Note that if Babel will be the only reason to implement this feature, we should wait until we are 100% sure that Babel will be released as ESM.

I think it makes sense regardless, if nothing else for the flexibility for people to download some remote stuff or something.

@ahnpnl
Copy link
Contributor

ahnpnl commented Feb 12, 2021

Also any information from "preload" should be passed to transformer instance.

I don't really like that. Maybe we should go with a class instead of a plain object? Then that class can stick whatever it wants as a property on its instances.

This looks OK to me. My request is only that transformer should be able to access the data of "preload" phase.

Currently I'm using this experimental workaround

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

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

@nicolo-ribaudo
Copy link
Contributor Author

Maybe we should go with a class instead of a plain object? Then that class can stick whatever it wants as a property on its instances.

Assuming that the class is only instantiated once (before actually running/compiling the tests), wouldn't it be the same as a normal variable outside the object?

let state;

export const transformer = {
   async initialize() {
     state = { foo: 2 }
   },

   process(sourceText, sourcePath, options) {
      return transform(sourceText, state);
   } 
 } 

@ahnpnl
Copy link
Contributor

ahnpnl commented Feb 12, 2021

Maybe we should go with a class instead of a plain object? Then that class can stick whatever it wants as a property on its instances.

Assuming that the class is only instantiated once (before actually running/compiling the tests), wouldn't it be the same as a normal variable outside the object?

let state;

export const transformer = {
   async initialize() {
     state = { foo: 2 }
   },

   process(sourceText, sourcePath, options) {
      return transform(sourceText, state);
   } 
 } 

I actually don't like this usage because it might end up that all workers do the same thing. My idea is something which loads stuffs once before initializing transformers. That will benefit more for performance.

@nicolo-ribaudo
Copy link
Contributor Author

nicolo-ribaudo commented Feb 12, 2021

That would only be possible if the result of initialize() is serializable/transferable across workers.
In my original usecase (doing babel = await import("@babel/core")), the result is not serializable (and can't be passed to a worker) because it is a bunch of functions.

@jeysal
Copy link
Contributor

jeysal commented Feb 12, 2021

How is this better than enabling async transformation? (#9889 was probably the latest on that topic?) The async initialization could be done first thing in process. It seems that if we were to implement preload, we'd ideally call it lazily when the first file to transform comes in, so it's no different than doing async work at the beginning of process.
Of course there may be cases where, unlike with import(), the async work actually takes significant time every time you run it, but I think transformer state would be good enough to solve that.

@nicolo-ribaudo
Copy link
Contributor Author

Async transformation doesn't work with require() (unless you do something like https://giuseppegurgone.com/synchronizing-async-functions/), because require() is synchronous.

@SimenB
Copy link
Member

SimenB commented Feb 12, 2021

How is this better than enabling async transformation? (#9889 was probably the latest on that topic?)

CJS is synchronous, so any async fork of the logic is a non-starter. Async transformation is orthogonal to allowing sync transformation to do some async preparation work.

However, ESM is async. So the question is if any use case for async code stransforms is covered by ESM - in that case this issue is not worth addressing. That said, I think "async setup before sync transformation" is a valid use case.

(unless you do something like giuseppegurgone.com/synchronizing-async-functions), because require() is synchronous.

I'd jump through 100 hoops to avoid hacking sync vs async. I get why Babel has gone that way, but I don't think it's necessarily worth it for others

@ahnpnl
Copy link
Contributor

ahnpnl commented Feb 12, 2021

I think transformer state would be good enough to solve that.

I like the transformer state idea. Transformer state will also help custom transformers to remove the caching logic. Currently ts-jest has to implement a caching logic to avoid recreating TypeScript compiler on each run https://github.com/kulshekhar/ts-jest/blob/master/src/ts-jest-transformer.ts#L59

@jeysal
Copy link
Contributor

jeysal commented Feb 12, 2021

Oh, yeah I didn't realize it was about transforming in a sync context where the code that does the import. I thought it was a long term thing for when we want to change to the Babel ESM version, but for CJS we would keep using the sync one

@SimenB
Copy link
Member

SimenB commented Mar 5, 2021

@nicolo-ribaudo is your use case being able to do import() or is there something more you want to do that is also async? I think I'll be adding support for using native ESM transformers (and other pluggable stuff) for the next major, and if the use case is solely @babel/core being ESM then that should be covered by ESM support. But if there are use cases for doing other async stuff we should probably still implement something for a prepare or whatever

@nicolo-ribaudo
Copy link
Contributor Author

I think import() will be enough 馃憤

@SimenB
Copy link
Member

SimenB commented Mar 6, 2021

@nicolo-ribaudo #11163

@SimenB
Copy link
Member

SimenB commented Mar 6, 2021

That diff also makes it trivial to call some async prepare if needed

@SimenB
Copy link
Member

SimenB commented Mar 8, 2021

Released in next.4

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Mar 8, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions
Copy link

github-actions bot commented May 8, 2022

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 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants