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

Ignore rootDir to determine the cache key of transformed files in babel-jest #7947

Closed

Conversation

rubennorte
Copy link
Contributor

Summary

Removes the root directory from the determination of the cache key in babel-jest.

The cache directory used by Jest depends on the root directory so different projects store their artifacts in different caches. With that guarantee, we don't need to take the root directory into account to compute the cache key of the transformed files, so we can store them remotely and restore them in different hosts/locations.

Test plan

Added a test to verify this behaviour.

In terms of performance, we crawl the filesystem by default to find Babel configuration files and that's an order of magnitude higher than the additional processing we're doing with this change.

@rubennorte rubennorte force-pushed the ignore-root-dir-in-babel-jest-cache branch 2 times, most recently from 3a5f5ea to b653405 Compare February 21, 2019 17:57
.update(JSON.stringify(babelOptions.options))
.update(
// Not pretty but not coupled to a specific signature of the
// babel options to relativize paths
Copy link
Contributor Author

@rubennorte rubennorte Feb 21, 2019

Choose a reason for hiding this comment

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

Thoughts on this? 🤷🏻‍♂️

packages/babel-jest/package.json Show resolved Hide resolved
packages/babel-jest/src/__tests__/index.ts Show resolved Hide resolved
Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

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

Let's use the replacer argument of JSON.stringify to do the transform on-the-fly while serializing. The current approach is hacky, and might fail for paths containing weird characters (e.g. a double quote, which will appear in JSON as \\"; or in Windows, where the backslash is double-escaped (i.e. \\).

@rubennorte rubennorte force-pushed the ignore-root-dir-in-babel-jest-cache branch 2 times, most recently from 9e8b58e to 47ffedb Compare February 21, 2019 18:29
@SimenB
Copy link
Member

SimenB commented Feb 21, 2019

@mjesun does this lgty?

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.

New test is failing on Windows

Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

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

LGTM. Let's get rid of the ugly $1, though.

packages/babel-jest/src/index.ts Outdated Show resolved Hide resolved
// babel options to relativize paths
return JSON.stringify(babelOptions.options, (_key, value) => {
if (typeof value === 'string') {
return value.replace(rootDirRegExp, '<rootDir>$1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return value.replace(rootDirRegExp, '<rootDir>$1');
return value.replace(rootDirRegExp, '<rootDir>');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I thought lookahead wasn't supported in Node <9 but that was lookbehind 😅😅

@SimenB
Copy link
Member

SimenB commented Feb 22, 2019

Bah, ignore the node 6 failure, I'll fix it tomorrow. syntax error in a babel plugin (trailing function commas), nothing major

@rubennorte
Copy link
Contributor Author

@SimenB no worries, there's no rush. I can fix it tomorrow.

@rubennorte rubennorte force-pushed the ignore-root-dir-in-babel-jest-cache branch from ba8e8bb to f9d0ca6 Compare February 22, 2019 10:59
@rubennorte rubennorte force-pushed the ignore-root-dir-in-babel-jest-cache branch from f9d0ca6 to 3aacfa0 Compare February 22, 2019 11:19
@rubennorte
Copy link
Contributor Author

rubennorte commented Feb 22, 2019

After some internal testing we've decided not to go forward with this approach. We've found that there are some cases where parent directories of the rootDir appear in the babel configuration, so any regexp based approach wouldn't work. We could explicitly check the paths we're aware of and use path.relative but there are some cases where this wouldn't work either (untyped plugin options, etc.).

We were using babel-jest so we had support for nested .babelrc files. We'll implement a custom transform and get rid of those, so we can:

  1. Have a centralized babel configuration that we can use to invalidate the cache when the transform changes.
  2. Avoid crawling the file system for each file being processed (even to get the cache key!)

If we wanted to fix this in babel-jest it'd require banning nested .babelrc files and forcing users to use overrides in a single babel configuration we could read once for all files being transformed, but I don't think that's the right thing to do for the general user.

Sorry guys I wasted your time with this one (at least it was useful to find #7945).

@rubennorte rubennorte closed this Feb 22, 2019
@rubennorte rubennorte deleted the ignore-root-dir-in-babel-jest-cache branch February 22, 2019 15:56
@SimenB
Copy link
Member

SimenB commented Feb 22, 2019

  1. Avoid crawling the file system for each file being processed (even to get the cache key!)

Could babel improve their algorithm? Seems silly to crawl the FS e.g. for sibling files - you'll find the same thing (barring ignores etc, but that should be in memory as well, so no need to hit the FS if you know the same config will apply)

@rubennorte
Copy link
Contributor Author

I assume they do have an in-memory cache for that, but that's still not great for our use case. It's also not our main blocker (we knew about it and could've accepted the consequences).

@SimenB
Copy link
Member

SimenB commented Feb 22, 2019

So they don't actually crawl for every single file? (sorry, just trying to understand)

@rubennorte
Copy link
Contributor Author

I'm not an expert but I see some caching mechanisms here: https://github.com/babel/babel/blob/master/packages/babel-core/src/config/config-chain.js

I'd say the correct thing to do is to have an in-memory cache of the babel configuration files and then merge and apply for each file, but I haven't checked it's actually what it does.

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

5 participants