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

Allow JSON transforms #8278

Merged
merged 11 commits into from
Apr 11, 2019
Merged

Conversation

MLoughry
Copy link
Contributor

@MLoughry MLoughry commented Apr 5, 2019

Summary

Fixes #2578

Previously, jest would not honor custom transformations for JSON files, despite the documentation not mentioning anything of the sort. Per @cpojer, this was done because there were no use-cases for transforming JSON files; however, my project has a webpack plugin that does JSON transforms, and not having the same capability in jest makes testing less intuitive and more difficult.

Test plan

Added tests to packages\jest-runtime\src\__tests__\runtime_internal_module.test.js demonstrating that transforms work on .json modules, but are not applied to internal JSON modules.

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

@scotthovestadt
Copy link
Contributor

scotthovestadt commented Apr 5, 2019

@MLoughry I'm 100% on-board with allowing JSON transforms. I do think there is an open question about whether the transformer should be given the raw string or a JS object to transform.

Do you have a use-case for needing the string? My instinct is that it should take an object and return an object. Few random reasons I feel that way:

  • I want the error boundaries to be clear. If the transformer fails, that's one error. If the JSON file itself cannot be parsed at all, that's different.
  • If you load the JSON file via require, it comes to you as an object already. Obviously that's not happening today, but if we require the string to be loaded it may limit the future.

EDIT: see below

Once this interface exists it can be difficult to change it so I think we should consider this carefully. @SimenB @cpojer @thymikee thoughts?

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 5, 2019

I don't have strong opinions on what the contract/interface for the transformer should be. However, this is my first time working in Jest, and I'm entirely sure where else it would make sense to branch the logic in order to do as you suggest.

@scotthovestadt
Copy link
Contributor

@MLoughry I took a look at the code and I changed my mind. It's more consistent to work like this. It's just a little weird for JSON (since you're not parsing it with Babel or anything...)

Assuming tests pass I'll approve.

@scotthovestadt
Copy link
Contributor

@MLoughry Thanks for the PR!

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 5, 2019

Additionally, I'm investigating test failures from the latest CI . If I exclude this test, the others pass.

My assumption is that some require or transform cache is not being properly cleared in the test. Is there a way to force that? And/or does this indicate some non-test problem?

@scotthovestadt
Copy link
Contributor

@MLoughry I recommend you install this modified version of Jest (not very many files have changed, can do manually pretty easily) in your project and double check it all works for you before merge as well.

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

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

  • Can't lose the strip-bom functionality when adding transformer functionality.
  • Tests need to pass.

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 5, 2019

I had tested my first version of this change against my project, and it worked. However, I'm struggling trying to figure out why the tests are failing.

I'll try to dig in a bit more this weekend and Monday, but some guidance would be great. I'm not sure where/how to install a default transform, nor do I fully understand why it doesn't work without one.

@scotthovestadt
Copy link
Contributor

@MLoughry OK, so what's happening is that when you removed the JSON parse these JSON files are entering a code path meant for JavaScript files. It's making a lot of assumptions. Actually I'm a little surprised it works at all. It's essentially parsing them like they are scripts.

It does reveal what's likely a performance problem with loading JSON files, though. As far as I can tell we have no cache on it at all currently. So every time a JSON file is loaded it's going to hit the disk.

I think probably the best way to do this is to just keep the code path for JSON separate and just add your transform on top of that. Here's a rough version with performance issues:

private _loadModule(
  localModule: InitialModule,
  from: Config.Path,
  moduleName: string | undefined,
  modulePath: Config.Path,
  options: InternalModuleOptions | undefined,
  moduleRegistry: ModuleRegistry,
) {
  if (path.extname(modulePath) === '.json') {
    let text = stripBOM(fs.readFileSync(modulePath, 'utf8'));

    if (!options || !options.isInternalModule) {
      for (const [regex, file] of this._config.transform) {
        if (new RegExp(regex).test(modulePath)) {
          const transformer = require(file) as Transformer;
          text = transformer.process(text, modulePath, this._config) as string;
          break;
        }
      }
    }

    localModule.exports = this._environment.global.JSON.parse(text);
  } else if (path.extname(modulePath) === '.node') {
    localModule.exports = require(modulePath);
  } else {
    // Only include the fromPath if a moduleName is given. Else treat as root.
    const fromPath = moduleName ? from : null;
    this._execModule(localModule, options, moduleRegistry, fromPath);
  }
  localModule.loaded = true;
}

This works and tests seem to pass. It's not that great on performance, though, and I've duplicated code from ScriptTransformer.ts (to actually find the transformer). Here is what you can do to get it merged:

  1. Apply the code I pasted above and verify it works for you, functionally. Tests will pass.
  2. Pull the code I duplicated out of ScriptTransformer into it's own class. Transformers.ts? This should include calcTransformRegExp and basically provide a way to let you find a transformer by the filename. The main thing is making sure the RegExp instances get cached, you can see in my rough impl above we're reconstructing them every time. ScriptTransformer caches them internally but now we need to make that reusable for your JSON.
  3. Use the new Transformers class to eliminate the code duplication I've created (and fix some optimization issues).

Once you have that working the actual implementation should look something like this:

if (path.extname(modulePath) === '.json') {
  let text = stripBOM(fs.readFileSync(modulePath, 'utf8'));

  if (!options || !options.isInternalModule) {
    const transformer = this._transformers.forFile(modulePath);
    if (transformer) {
      text = transformer.process(text, modulePath, this._config) as string;
    }
  }
}

Let me know if you get stuck, happy to help.

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 6, 2019

Thanks for the pointers.

The tests seemed to be passing, aside from an out-of-memory exception in jest-circus on commit b4c5a1f that I can't repro locally. I made a small commit to update the Changelog to force a re-run of that CI check.

@scotthovestadt
Copy link
Contributor

Don't worry about the OOM, it's unrelated.

Copy link
Contributor

@scotthovestadt scotthovestadt left a comment

Choose a reason for hiding this comment

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

LGTM, want additional reviews to be sure I didn't miss anything vital. @SimenB?

Thanks for your work on this.

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.

I'm not sure I agree it makes sense to have this transform as a separate thing in the transformer. However, I don't have any better suggestions while keeping the node compat (where .json files are parsed to objects automatically)

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #8278 into master will decrease coverage by 0.03%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8278      +/-   ##
==========================================
- Coverage   62.14%   62.11%   -0.04%     
==========================================
  Files         266      266              
  Lines       10666    10676      +10     
  Branches     2590     2592       +2     
==========================================
+ Hits         6628     6631       +3     
- Misses       3448     3455       +7     
  Partials      590      590
Impacted Files Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts 77.77% <0%> (-3%) ⬇️
packages/jest-runtime/src/index.ts 69.63% <100%> (+0.24%) ⬆️

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 f435f01...701d3eb. Read the comment docs.

@scotthovestadt
Copy link
Contributor

scotthovestadt commented Apr 7, 2019

FYI: will be doing a patch release shortly with a minor fix to inline snapshots and a memory leak fix after I do some additional verification that neither is likely to break anything, and then this can be merged in and make the next non-patch release.

Hold off on merge for now.

@MLoughry
Copy link
Contributor Author

MLoughry commented Apr 7, 2019

What would be the expected timeline for releasing this change? I have an important (but luckily not urgent) migration project blocked behind this.

@scotthovestadt
Copy link
Contributor

We'll be starting work on breaking changes for Jest v25 soon and I'll make sure your change is pushed out with the last of Jest v24 before that happens. I expect that'll be about 2 weeks. There's another feature I'm working on that I want to go out in the same minor version, so that's a factor as well.

I'm slightly concerned about users with bad transform RegExp (eg '.*\\.js' instead of '.*\\.js$') but... well, I don't think this counts as a breaking change regardless. I might need to do a pass and make sure that there is a decent error message if that happens to someone.

@devinus
Copy link

devinus commented May 27, 2019

This still unfortunately makes the assumes you want a parsed JSON object back. There are a lot of e.g. Webpack loaders that turn JSON definitions into something entirely different. In my case, I'm turning JSON Schema definitions into validation functions.

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

JSON files are not transformed via custom preprocessor
7 participants