Transformer configs

[eightypop]

Summary

This allows a configuration object to be passed in via the jest config to a transformer in a format that is pretty standard and used by other projects like babel. Instead of just the path or the name of the package, an array could be specified, with the first index being the package and the second being the configuration.

transform: {
        '\\.js$': [ require.resolve('babel-jest'), { rootMode: 'upward' } ],
}

This solves a use case for it is necessary to configure something like babel-jest in order for babel to find its config. Specifically, when using babel 7 you might need to configure the rootMode option in order for babel to locate its configuration file.

Test plan

I've added unit tests and an e2e test that pass.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[jharris4]

Hi there! I made a similar PR to this one if you're curious: #7398

TLDR:
instead of transform: [transformResolvePath, transformConfig] I used transform: () => transformer

[rickhanlonii]

Very cool!

@jharris4 thanks for taking a look as well! I think the format here is more common, what do you think?

[jharris4]

@rickhanlonii Thanks for the feedback. I definitely have some thoughts! :-)

My motivation for looking at this is that I'm prototyping a JS build tool that lets you configure tools like babel/jest/rollup/webpack etc all in one place.

Tools like webpack & rollup use JavaScript (not JSON!) configs, so you can do:

const webpack = require('webpack');
const SomeWebpackPlugin = require('some-webpack-plugin');
const webpackConfig = { plugins: [new SomeWebpackPlugin({ options: 'go here' })] };
webpack(webpackConfig);

const rollup = require('rollup');
const SomeRollupPlugin = require('some-rollup-plugin');
const rollupConfig = { plugins: [new SomeRollupPlugin({ options: 'go here' })] };
rollup(rollupConfig);

This is nice and self contained and my tool can create a custom plugin inline if I want like this:

const MyWebpackPlugin = (webpackPluginOptions) => { results: 'webpack plugin results' };
const webpackConfig = { plugins: [new MyWebpackPlugin({ options: 'go here' })] };

I would argue that the above is the more common format.

A less common format is one where you can't use a plain JS config but are restricted to JSON (no functions allowed!).

With JSON format configs, you are forced to split your config into multiple files if you want to define any logic, and you have to rely on the tool to catch any errors loading those files instead of just import/require them yourself.

Babel and Jest are the only tools I'm aware of that still use this restriction.

I have yet to hear a great argument for why the config should be restricted to JSON. Specifically, what benefit is there to requiring that all imported logic for a tool has to be specified by a string resolve path to a module instead of just letting the module logic be included directly?

I have a hunch that it's a relic from the days where the standard way to configure tools was via a section in package.json or arguments passed as strings to the command line.

Babel have even taken a steps towards remedying this with version 7. They now also support a babelrc.js file.

[rickhanlonii]

All great thoughts - I'm now on the fence and want to do some more research to figure out which one I'd support (happy for someone else to make this decision)

One note:

what benefit is there to requiring that all imported logic for a tool has to be specified by a string resolve path to a module instead of just letting the module logic be included directly

The main benefit that comes to my mind is specifying the options via cli, which is v common particularly in CI environments

[jharris4]

I totally agree that specifying options via cli is an important feature of jest.

When the jest cli is run from the command line it actually calls require('./packages/jest-cli).run`.

This run function parses the command line arguments and then passes them to require('./packages/jest-cli).runCLI`.

The changes in the PR enhance the runCLI method to support a config as object argument in a backwards compatible way.

All the command line argument validation happens in the run function, so the jest cli will still work identically as before. (i.e. it will throw an error if you pass a non JSON parseable config).

The only difference is that I've opened a door to more complex configuration options.

Namely you can now configure jest without passing 'a-module-path' and having it call `require('a-module-path-string') for you.

I think this change makes a lot of sense, but I do find the function naming to be a little confusing. :-)

Maybe it would be clearer if I added some parallel runRawConfig function to complement run and runCLI ?

[jharris4]

Also fwiw this PR also looks great even though it has slightly different goals from mine.

I don't think it would be too hard to merge them with each other if we wanted to.

Should probably move the conversation over to #7398 :-)

[eightypop]

@jharris4 interesting PR. I'll take a closer look, but I think we are trying to accomplish two different things. This PR allows a transformer to be initialized with some jest-specific config, which is something that's not supported currently.

As far as the case for specifying the module and config separately, babel-jest provides one. If I wanted to write my own transformer I could, but I'd rather reuse the existing babel-jest package and then configure it slightly for my use case.

I think the difference between JSON and Javascript configs with regards to the future of Jest is a separate and larger conversation and this PR doesn't really express an opinion on that matter.

[yordis]

This implementation is what I was thinking after I ran into the issue of serializing between workers (I didn't know 😄)

So I was literally thinking on changing my PR to this style since most of the tools out there use an array of 2 elements for these configs.

[bradfordlemley]

This PR adds the functionality I am looking for, thanks!

For a reference case in the wild, see how react-scripts currently needs a dedicated module just to do some minor babel config for jest. With this PR, that dedicated module won't be necessary, since the configuration can be inline. Other react-scripts-like tools run into the same issue.

Transform via function (#7398) would also be powerful. It would also solve the issue, but at the expense of not being able to serialize the config, as already described above. Supporting both options would be great!

[eightypop]

@rickhanlonii Do you think any further discussion or work is required before we can merge this?

[SimenB]

I have yet to hear a great argument for why the config should be restricted to JSON.

It must be JSON serializable so we can send it between workers. Might be fixed if we're using worker threads? Not sure

[SimenB]

@whatknight this is awesome, thanks! Sorry about the slow process.

Could you rebase this and handle the comment I left inline? Happy to merge at that point 🙂

[SimenB]

Left inline comment.

Also, please update the changelog 🙂

[SimenB]

Sorry, I got no email or notification that you had updated! 😅

[codecov-io]

Codecov Report

Merging #7288 into master will increase coverage by 0.07%.
The diff coverage is 94.73%.

@@            Coverage Diff            @@
##           master   #7288      +/-   ##
=========================================
+ Coverage   63.32%   63.4%   +0.07%     
=========================================
  Files         273     274       +1     
  Lines       11327   11342      +15     
  Branches     2768    2771       +3     
=========================================
+ Hits         7173    7191      +18     
+ Misses       3536    3534       -2     
+ Partials      618     617       -1

Impacted Files	Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts	78.35% <100%> (+0.45%)	⬆️
...former-config/this-directory-is-covered/Covered.js	80% <80%> (ø)
packages/jest-config/src/normalize.ts	79.87% <96.15%> (+1.66%)	⬆️

---

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 b644d85...94c9a74. Read the comment docs.

[thymikee]

I wonder if we pay any perf penalty for making this an array of variable length (previously always a tuple, now an array of 2 or 3 items). It's accessed before every file transform, so definitely something worth double-checking.

[eightypop]

wouldn't it be uniform in most cases that are using only one config file?

[SimenB]

@whatknight ping 🙂

[eightypop]

@SimenB I'll try to get to this tonight.

[SimenB]

@whatknight I gave a (few) merges a try since this has been lingering for so long. Feel free to revert them if it turned out wrong 🙂

[SimenB]

@jeysal is this something you know how to fix? 😀

[SimenB]

worked without split on 3.4 (I forgot to run yarn).

This wonderful error on 3.5

Essentially, since the index signature does not match, it gets super confused even though we'll only be merging 1 at a time

[SimenB]

Let's land this before there are any more conflicts.

Thank you so much for your patience @whatknight, sorry it took so long!

[pedrottimark]

@SimenB FYI after yarn jest today, there are changes to e2e/transform/transformer-config/yarn.lock which I had to discard before I opened a pull request

[SimenB]

Ah, thanks! I'll push it to master.

EDIT: bb01c94

[github-actions]

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.