feat(jest-runtime): share cacheFS between runtime and transformer by ahnpnl · Pull Request #10901 · jestjs/jest

ahnpnl · 2020-12-02T11:19:20Z

Summary

Pass cacheFS from jest-runtime to ScriptTransformer. When getCacheKey or process is invoked, this cacheFS is passed in through transform options

If a transformer does module resolution and reads files, it should populate cacheFS so that Jest avoids reading the same files again, improving performance. cacheFS stores entries of <file path, file contents>

Close #10898

Test plan

Added unit test for jest-transform

ahnpnl · 2020-12-02T17:33:53Z

Node 14.x Windows, Node 12.x MacOS, Node LTS MacOS and facebook.jest failed but I have no idea what causes it.

SimenB · 2020-12-03T08:59:01Z

Node 14.x Windows, Node 12.x MacOS, Node LTS MacOS and facebook.jest failed but I have no idea what causes it.

macOS is flaky: #10828

SimenB

I wonder if we should pass it in the constructor rather than in the individual transform calls?

ahnpnl · 2020-12-03T09:01:16Z

Btw I could only modify test for jest-transform. For jest-runtime, I don't know which tests I can change to test the passing of cacheFS to ScriptTransformer.transform.

ahnpnl · 2020-12-03T09:02:42Z

I wonder if we should pass it in the constructor rather than in the individual transform calls?

I tried with constructor. However, when passing through constructor, cacheFS isn't updated for each test run.

SimenB · 2020-12-03T09:06:07Z

What do you mean "updated"?

ahnpnl · 2020-12-03T09:10:58Z

What do you mean "updated"?

For example, a simple project:

When custom transformer is initialized, cacheFS has 100 elements
Test file foo.spec.ts is processed. After processing, cacheFS in custom transformer shows 200 elements.
A normal file foo.ts comes into custom trasnformer to be processed. However, cacheFS before processing this new file at this time shows 100 elements, which is the initial amount.

ahnpnl · 2020-12-03T09:26:16Z

hmm it seems like possible to pass through the constructor. I think I made mistakes somewhere previously

SimenB

this should update https://github.com/facebook/jest/blob/master/docs/CodeTransformation.md as well, other than that this LGTM 👍

ahnpnl · 2020-12-03T09:59:12Z

 interface Transformer<OptionType = unknown> {
  /**
   * Indicates if the transformer is capabale of instrumenting the code for code coverage.
+   * - `cacheFS`: if custom transformers do module resolution and read file, custom transformers should populate this


@SimenB please check :)

SimenB

thanks!

thymikee

Nice!

SimenB · 2020-12-04T11:51:24Z

Hmm, I wonder if it makes more sense to pass as part of TransformOptions rather than forcing people to implement createTransformer?

ahnpnl · 2020-12-04T11:57:24Z

Passing to getCacheKey transform options is also ok to me. Indeed if not implementing createTransformer, you won't get that cacheFS so it forces yes.

On the other hands, this cacheFS is partially related to transform options. It's related to transformer actually. Transform options to me is something directly related to how codes are transformed while this cacheFS is not directly related.

SimenB · 2020-12-04T12:01:41Z

Right, but createTransformer is also a "public" API (in that user can be expected to run it themselves) at which point that transformer won't get cacheFs passed. See e.g. https://jestjs.io/docs/en/tutorial-react#custom-transformers

ahnpnl · 2020-12-04T12:09:02Z

Hmm indeed, seem like we need to pass it as transform options then, unless others have other ideas

ahnpnl · 2020-12-04T22:08:57Z

Passing into getCacheKey as the last arg brings more benefits over extending TransformOption:

Ensure that this arg is mandatory while passing via transform option requires cacheFS to be an optional property otherwise jest-repl will complain
Since it’s not directly related to how codes are transformed, passing as independent arg ensures that.
Users don’t have to be forced to implement create transformer function, as well as cover the case “public” API

Pass `cacheFS` from `jest-runtime` to `ScriptTransformer`. When `getCacheKey` or `process` is invoked, this `cacheFS` is passed in through transform options If a transformer does module resolution and reads files, it should populate `cacheFS` so that Jest avoids reading the same files again, improving performance. `cacheFS` stores entries of <file path, file contents> Closes #10898

SimenB

thanks!

github-actions · 2021-05-10T23:07:07Z

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.

facebook-github-bot added the cla signed label Dec 2, 2020

ahnpnl changed the title ~~feat(runtime): share cacheFS between runtime and transformer~~ feat(jest-runtime): share cacheFS between runtime and transformer Dec 2, 2020

ahnpnl marked this pull request as ready for review December 2, 2020 17:32

ahnpnl commented Dec 2, 2020

View reviewed changes

Comment thread packages/jest-transform/src/types.ts Outdated

SimenB approved these changes Dec 3, 2020

View reviewed changes

Comment thread packages/jest-transform/src/ScriptTransformer.ts Outdated

SimenB reviewed Dec 3, 2020

View reviewed changes

Comment thread packages/jest-transform/src/ScriptTransformer.ts Outdated

SimenB approved these changes Dec 3, 2020

View reviewed changes

ahnpnl commented Dec 3, 2020

View reviewed changes

SimenB reviewed Dec 3, 2020

View reviewed changes

Comment thread docs/CodeTransformation.md Outdated

SimenB reviewed Dec 3, 2020

View reviewed changes

Comment thread docs/CodeTransformation.md Outdated

SimenB approved these changes Dec 3, 2020

View reviewed changes

SimenB requested review from jeysal and thymikee December 3, 2020 11:01

thymikee approved these changes Dec 3, 2020

View reviewed changes

Comment thread docs/CodeTransformation.md Outdated

ahnpnl requested review from SimenB and thymikee December 4, 2020 21:56

SimenB reviewed Dec 5, 2020

View reviewed changes

Comment thread docs/CodeTransformation.md Outdated

Update CodeTransformation.md

c389808

SimenB approved these changes Dec 5, 2020

View reviewed changes

SimenB merged commit e7ad911 into jestjs:master Dec 5, 2020

ahnpnl deleted the share-cachefs branch December 5, 2020 12:06

github-actions Bot locked as resolved and limited conversation to collaborators May 10, 2021

Uh oh!

Conversation

ahnpnl commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

ahnpnl commented Dec 2, 2020

Uh oh!

Uh oh!

SimenB commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahnpnl commented Dec 3, 2020

Uh oh!

Uh oh!

ahnpnl commented Dec 3, 2020

Uh oh!

SimenB commented Dec 3, 2020

Uh oh!

ahnpnl commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahnpnl commented Dec 3, 2020

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

ahnpnl Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimenB commented Dec 4, 2020

Uh oh!

ahnpnl commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimenB commented Dec 4, 2020

Uh oh!

ahnpnl commented Dec 4, 2020

Uh oh!

ahnpnl commented Dec 4, 2020

Uh oh!

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahnpnl commented Dec 2, 2020 •

edited

Loading

SimenB commented Dec 3, 2020 •

edited

Loading

ahnpnl commented Dec 3, 2020 •

edited

Loading

ahnpnl commented Dec 4, 2020 •

edited

Loading