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

Transformer configs #7288

Merged
merged 27 commits into from Jun 26, 2019
Merged

Transformer configs #7288

merged 27 commits into from Jun 26, 2019

Conversation

eightypop
Copy link
Contributor

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
Copy link
Contributor

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 cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

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

@jharris4
Copy link

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
Copy link
Member

Very cool!

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

@jharris4
Copy link

@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
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Contributor Author

eightypop commented Nov 24, 2018

@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
Copy link
Contributor

yordis commented Nov 27, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@SimenB
Copy link
Member

SimenB commented Dec 25, 2018

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
Copy link
Member

SimenB commented Dec 25, 2018

@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 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left inline comment.

Also, please update the changelog 🙂

@SimenB SimenB added this to the Jest 24 milestone Dec 26, 2018
@SimenB
Copy link
Member

SimenB commented Jan 2, 2019

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

@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

packages/jest-config/src/__tests__/normalize.test.js Outdated Show resolved Hide resolved
e2e/transform/transformer-config/package.json Outdated Show resolved Hide resolved
);
expect(options.transform).toEqual([
[DEFAULT_CSS_PATTERN, '/root/node_modules/jest-regex-util'],
[DEFAULT_JS_PATTERN, require.resolve('babel-jest'), {rootMode: 'upward'}],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SimenB
Copy link
Member

SimenB commented Jan 10, 2019

@whatknight ping 🙂

@eightypop
Copy link
Contributor Author

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

@SimenB
Copy link
Member

SimenB commented Jun 26, 2019

@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 🙂

@@ -46,19 +46,29 @@ type AllOptions = Config.ProjectConfig & Config.GlobalConfig;
const createConfigError = (message: string) =>
new ValidationError(ERROR, message, DOCUMENTATION_NOTE);

const mergeOptionWithPreset = (
// TS 3.5 forces us to split these into 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This wonderful error on 3.5
image

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

@SimenB
Copy link
Member

SimenB commented Jun 26, 2019

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

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

@SimenB SimenB merged commit 47da9cf into jestjs:master Jun 26, 2019
@pedrottimark
Copy link
Contributor

@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
Copy link
Member

SimenB commented Jul 4, 2019

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

EDIT: bb01c94

@github-actions
Copy link

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.

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

Successfully merging this pull request may close these issues.

None yet