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

Add compile() method to complement process() for transformers #7018

Closed
jdalton opened this issue Sep 21, 2018 · 29 comments
Closed

Add compile() method to complement process() for transformers #7018

jdalton opened this issue Sep 21, 2018 · 29 comments

Comments

@jdalton
Copy link
Contributor

jdalton commented Sep 21, 2018

馃殌 Feature Proposal

Add support for a compile() like method to complement process() for transformers.

Motivation / Pitch

I'm writing an esm loader and trying to make it work as the ESM support layer for Jest (without using Babel). I found I could make esm provide a process() method and be used as a "transform": "esm" which is great! However, esm anchors its runtime to the module object created and ran through the wrapper(). Normally this isn't a problem, since esm owns the implementation it handles this in Module#compile or Module#_extensions. However, Jest constructs modules from scratch so there is no place for esm to hook into and decorate the module object. A hook like compile or init or beforeCompile would be one way of enabling this.

Related to #4842 (comment).

@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

I don't fully understand what you're asking for here. Do you want a hook into the runtime? What exactly is the current API missing? Do you need to execute some code instead of working on source code (text only)?

Also, do you have an API in mind?

@jdalton
Copy link
Contributor Author

jdalton commented Sep 21, 2018

@SimenB

Currently, at module construction, Jest will detect and run the transformer, here. This allows esm to be used as a transformer (producing the transformed ESM to instrumented CJS source). The transformed source is then used to create the compiled module script and then the module is executed.

Currently, a transformer can have the following properties/methods

  • transformer.getCacheKey()
  • transformer.process()
  • transformer.canInstrument (boolean)

I'm asking for another method that a transformer could provide that is called as a interceptor to the

https://github.com/facebook/jest/blob/c6512ad1b32a5d22aab9937300aa61aa87f76a27/packages/jest-runtime/src/index.js#L578-L591

call. This would allow decorating the localModule and any of the other things passed to the wrapper when it is invoked.

@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

So we invoke some function (compile or whatever) with the same args, then apply the return value to our current .call?

@jdalton
Copy link
Contributor Author

jdalton commented Sep 21, 2018

@SimenB

The transformer.process() method is invoked with
https://github.com/facebook/jest/blob/c6512ad1b32a5d22aab9937300aa61aa87f76a27/packages/jest-runtime/src/script_transformer.js#L220-L222

I imagine a compile() or complete() method could be called with

transformer.complete(wrapper,
  localModule,
  filename,
  this._environment.global,
  this._createJestObjectFor( 
     filename, 
     require: LocalModuleRequire
  )
)

This would provide access to the wrapper function and many of the vars needed to execute it. In the example above I provided a subset of args since the others can be gotten by way of the ones provided (but you could provide the exact args for flexibility down the line). The complete() method would be responsible for invoking the wrapper and returning the result. That gives a lot of freedom to the complete() method.

@SimenB
Copy link
Member

SimenB commented Sep 22, 2018

Thanks for the example!

I don't think it makes sense to stick this on transformer (it's meant to work on source code, its result is cached, you can run different transforms on different files). However, a new option like executeScripts or something might make sense, which has the responsibility to execute the modulewrapper function.

API-wise I like your suggestion. Only thing I would change is to pass an object instead of 5 arguments - I find it easier to use when I don't have to check the docs or implementation for positional arguments (although good IDEs mitigate this).

@thymikee @rickhanlonii @mattphillips thoughts on this?

@thymikee
Copy link
Collaborator

No clever thoughts on that, but it sounds like it would be a part of transforming pipeline, as it hooks into transformer results. And it could ship as a single transform, instead of asking users to set 2 separate options.

@mattphillips
Copy link
Contributor

I agree with @thymikee it would be nice to not ask for 2 separate options if possible.

I don't know much about this part of Jest's internals so not sure I can make a great recommendation on this but whatever we go with I'm happy to help and learn more :)

@jdalton
Copy link
Contributor Author

jdalton commented Sep 22, 2018

@SimenB

API-wise I like your suggestion. Only thing I would change is to pass an object instead of 5 arguments - I find it easier to use when I don't have to check the docs or implementation for positional arguments (although good IDEs mitigate this).

Ya, that's cool too. In my own unrelated code I use a wrapper pattern which is originalFunc, args. This is nice because if there's a code path to just defer to the original it can be as easy as Reflect.apply(originalFunc, args). Depending on if the API ends up more of a wrapper that could be a way with it.

As @thymikee mentions the method could end up being another property on the result of transformer.process() (currently the result has a .code property but it could also have a complete() method too).

@SimenB
Copy link
Member

SimenB commented Sep 22, 2018

It's not a transform though. transform today is sourceCode -> [newSourceCode, sourcemap], while this new feature wants to hook into the actual code and execution/runtime of it.

How would I be able to use JSX with esm as loader? Would it delegate to babel-jest? What about typescript? Or Vue?

I don't know anything about how esm works (does it insert runtime checks? work on AST? extract some parts and execute it?), so maybe I'm misunderstanding.

Use case for a working babel transform, but still the goodness of esm: https://twitter.com/dan_abramov/status/935160018508369920

@jdalton
Copy link
Contributor Author

jdalton commented Sep 22, 2018

@SimenB

Maybe a shift if terms is needed. Instead of transform it could be seen as a module construction pipeline with hooks to process source code, wrapper arguments, etc.

How would I be able to use JSX with esm as loader? Would it delegate to babel-jest? What about typescript? Or Vue?

In those cases you're wanting to use a generic transpiler so likely sticking with babel-jest. For others that just want ESM without a lot of fuss using the esm loader is appealing.

I don't know anything about how esm works

The esm enables ESM syntax in Node. It's an application level loader, with node -r esm, as well as an isolated per package loader, via require = require("esm")(module).

By exposing a process() method on the esm export I can tap into the transform bit for source but still would need a way to access the localModule before execution as normally in the esm version of Module#compile I have access to it. The esm loader inside its Module#compile will decorate the localModule with a reference to the esm runtime library that the transpiled source will use. Having access to the arguments before they are executed in wrapper.call() would resolve that.

Adding another hook doesn't invalidate or break Babel. If the word transform is throwing you then maybe a shift in terms is needed. It could also be seen as lifecycle hooks for the module parse (transform), instantiate, execute.

@SimenB
Copy link
Member

SimenB commented Sep 22, 2018

Could you post how (ish) you'd implement the transformer? Both process and a new compile. Might help my understanding a bit :)

@jdalton
Copy link
Contributor Author

jdalton commented Sep 23, 2018

@SimenB Sure!

function process(source, filename) {
  const { code } = ESMCachingCompiler.compile(source, { filename, ...})

  return {
    code,
    complete(wrapper, args) {
     const [mod] = args

     ESMRuntime.enable(mod, {})
     return Reflect.apply(wrapper, args)
  }
}

@SimenB
Copy link
Member

SimenB commented Sep 23, 2018

Ah, that's pretty neat!

@cpojer @aaronabramov do you have any thoughts on the API here? I would still like for this to be possible to use together with e.g. Babel, but if this is an OK first go we should do it.

We could also go crazy and have an --esm flag so it's provided out of the box (like --coverage is). Would be coupled to this implementation, but I think that's fine?

@jdalton
Copy link
Contributor Author

jdalton commented Sep 25, 2018

@SimenB

do you have any thoughts on the API here? I would still like for this to be possible to use together with e.g. Babel, but if this is an OK first go we should do it.

I think that would be more of allowing multiple transformers per pattern match. A new loader option could be created for more loader specific hooks (so more than just transform) but layering would still be a thing to tackle.

We could also go crazy and have an --esm flag so it's provided out of the box (like --coverage is). Would be coupled to this implementation, but I think that's fine?

I'd like a more generic approach if possible, instead of a fixed/hard-coded approach. Jest handles the entire module construction so some hook into it, like a complete or compile, can open the door to more than just the esm loader.

@cpojer
Copy link
Member

cpojer commented Sep 25, 2018

I'm not convinced adding a feature to transformers solely to support one type of transformer is a good idea.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 25, 2018

@cpojer

Hooks to various phases of the Jest module construction and execution can be generally useful without being specific or hardcoded for a single transformer.

@cpojer
Copy link
Member

cpojer commented Sep 25, 2018

Do we have 3-5 other real use cases for a hook like that?

@jdalton
Copy link
Contributor Author

jdalton commented Sep 25, 2018

@cpojer

The loaders I'm aware of are @babel/register, esm, and reify.

There are also packages that tap into

  • Module#compile
  • Module._extensions
  • Module._load
  • Module._resolveFilename
  • Module._findPath

and other CJS module methods which may or may not work with Jest since Jest constructs modules in a way that doesn't leverage the traditional module creation/execution process.

Update:

Without a hook here is the way I've got to tackle this...

const hooked = { __proto__: null }

makeRequireFunction.process = (content, filename, { testEnvironment }) => {
  const EVAL_RESULT_VARIABLE = "Object.<anonymous>"
  const Environment = require(testEnvironment)
  const { runScript } = Environment.prototype

  if (! hooked[testEnvironment]) {
    hooked[testEnvironment] = true
    Environment.prototype.runScript = function (...args) {
      const result = Reflect.apply(runScript, this, args)
      const wrapper = result[EVAL_RESULT_VARIABLE]

      result[EVAL_RESULT_VARIABLE] = function (...args) {
        const [mod] = args
        /* hook it here */
        return Reflect.apply(wrapper, this, args)
      }

      return result
    }
  }

  /* transform code */
  return { code }
}

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

Without a hook here is the way I've got to tackle this...

Interesting, where would you put that code?

@jdalton
Copy link
Contributor Author

jdalton commented Sep 28, 2018

@SimenB

That code will be-in/or-exposed-in esm/esm.js, the main entry for esm, so that users can do

"transform": {
   "\\.m?js$": "esm"
}

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

Ah, gotcha! That will not work with a docblock setting the env then, as that overrides what's in globalConfig (which is the third argument). See https://jestjs.io/docs/en/configuration#testenvironment-string. Should work in the normal case, though! 馃檪

One possibility might be to create jest-environment-node-esm and jest-environment-jsdom-esm, then you can implement runScript yourself the way you want. At least as some sort of proof of concept. Doesn't scale well either of course, as that essentially means custom envs must also expose a version extending those 2.

@jdalton
Copy link
Contributor Author

jdalton commented Sep 28, 2018

@SimenB

That will not work with a docblock setting the env then, as that overrides what's in

That's good to know. I could sniff for the doc comment I guess.

One possibility might be to create jest-environment-node-esm and jest-environment-jsdom-esm

Ya, naw. I don't think I'll be doing that. The workaround for not having an exposed hook is just that, a workaround. I'm not thrilled about it, but until there's something better it's a way to unblock support and let devs get on their way.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2018

Yeah, definitely! Getting something out would be super awesome indeed.

@thymikee
Copy link
Collaborator

@jdalton seeing this #4842 (comment) are you still interested in proposed feature, or maybe transformer is good enough?

@jdalton
Copy link
Contributor Author

jdalton commented Dec 18, 2018

Hi @thymikee!

My use of transform is a hack-around for not having a supported way to wire-up the loader. Ideally l'd like a less brittle, more solid, way to do it.

@thymikee
Copy link
Collaborator

Ok, cool! I'm just not sure if we squeeze this in for v24. But since this is non breaking, I guess we can give it some more time.

@SimenB SimenB removed this from the Jest 24 milestone Jan 11, 2019
@github-actions
Copy link

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 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

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 1, 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 1, 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

5 participants