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

feat(jest-runtime): share cacheFS between runtime and transformer #10901

Merged
merged 2 commits into from Dec 5, 2020
Merged

feat(jest-runtime): share cacheFS between runtime and transformer #10901

merged 2 commits into from Dec 5, 2020

Conversation

ahnpnl
Copy link
Contributor

@ahnpnl ahnpnl commented Dec 2, 2020

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 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 ahnpnl marked this pull request as ready for review December 2, 2020 17:32
@ahnpnl
Copy link
Contributor Author

ahnpnl commented Dec 2, 2020

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

@SimenB
Copy link
Member

SimenB commented Dec 3, 2020

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

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 wonder if we should pass it in the constructor rather than in the individual transform calls?

packages/jest-transform/src/ScriptTransformer.ts Outdated Show resolved Hide resolved
@ahnpnl
Copy link
Contributor Author

ahnpnl commented Dec 3, 2020

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

ahnpnl commented Dec 3, 2020

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

SimenB commented Dec 3, 2020

What do you mean "updated"?

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Dec 3, 2020

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

ahnpnl commented Dec 3, 2020

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

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.

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

@@ -25,12 +25,14 @@ You can write you own transformer. The API of a transformer is as follows:
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB please check :)

docs/CodeTransformation.md Outdated Show resolved Hide resolved
docs/CodeTransformation.md Outdated Show resolved Hide resolved
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.

thanks!

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Nice!

docs/CodeTransformation.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Dec 4, 2020

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

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Dec 4, 2020

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

SimenB commented Dec 4, 2020

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

ahnpnl commented Dec 4, 2020

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

@ahnpnl
Copy link
Contributor Author

ahnpnl commented Dec 4, 2020

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

docs/CodeTransformation.md Outdated Show resolved Hide resolved
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
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.

thanks!

@SimenB SimenB merged commit e7ad911 into jestjs:master Dec 5, 2020
@ahnpnl ahnpnl deleted the share-cachefs branch December 5, 2020 12:06
@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 10, 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.

jest-runtime, jest-transform: share cacheFS between runtime and transformer
4 participants