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.

Sign up for GitHub

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

Replace colons in transform cache filenames #8353

Merged
merged 8 commits into from
Apr 30, 2019
Merged

Replace colons in transform cache filenames #8353

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
051c575
Replace colons in transform cache filenames
etr2460 Apr 21, 2019
70067fb
Merge branch 'master' into fix-cache-file-colons
Apr 21, 2019
6f47664
Include filename in cache key
etr2460 Apr 21, 2019
0737f6b
Remove all special characters from filename prefix
etr2460 Apr 22, 2019
57dd825
Merge branch 'master' into fix-cache-file-colons
etr2460 Apr 22, 2019
8b9d171
Merge branch 'master' into fix-cache-file-colons
etr2460 Apr 24, 2019
944291c
Update CHANGELOG.md
etr2460 Apr 24, 2019
af36665
Merge branch 'master' into fix-cache-file-colons
SimenB Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
- `[jest-haste-map]` Fix the `mapper` option which was incorrectly ignored ([#8299](https://github.com/facebook/jest/pull/8299))
- `[jest-jasmine2]` Fix describe return value warning being shown if the describe function throws ([#8335](https://github.com/facebook/jest/pull/8335))
- `[jest-environment-jsdom]` Re-declare global prototype of JSDOMEnvironment ([#8352](https://github.com/facebook/jest/pull/8352))
- `[jest-transform]` Replace colons in transform cache filenames to support Windows ([#8353](https://github.com/facebook/jest/pull/8353))

### Chore & Maintenance

Expand Down
10 changes: 6 additions & 4 deletions packages/jest-transform/src/ScriptTransformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export default class ScriptTransformer {
rootDir: this._config.rootDir,
}),
)
.update(filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary because getCacheKey includes the filename already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I thought so as well, my comment was only to seek confirmation ^^

.update(CACHE_VERSION)
.digest('hex');
} else {
Expand All @@ -102,6 +103,7 @@ export default class ScriptTransformer {
.update(fileData)
.update(configString)
.update(instrument ? 'instrument' : '')
.update(filename)
.update(CACHE_VERSION)
.digest('hex');
}
Expand All @@ -121,11 +123,11 @@ export default class ScriptTransformer {
// Create sub folders based on the cacheKey to avoid creating one
// directory with many files.
const cacheDir = path.join(baseCacheDir, cacheKey[0] + cacheKey[1]);
const cacheFilenamePrefix = path
.basename(filename, path.extname(filename))
.replace(/:/g, '_COLON_');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is colon really the only character that might cause issues here? Especially when you consider it's possible for a cache to be created on one system and then be used on another (this is what we do at Facebook and I'm sure it's not totally unique).

Stepping back a second, the extension being added to the file seems odd to me in the first place. With your .update(filename) added above, I don't see a reason we can't just drop this entire find/replace and then also stop saving the file with the extname prefix. The cache key has the whole filename in it. That'll do.

@SimenB let me know if you have any thoughts but here is my recommended course of action:

  1. Remove .update(filename) from the custom cache key code path since it has filename available already.
  2. Keep .update(filename) in the built-in cache key code path.
  3. Remove this find/replace and stop using the extname at all.

That should resolve the core issue and other similar issues we don't know about yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is obviously no technical necessity to have the prefix because it's already in the hash (and stripping away most of the path would make it useless for collision avoidance anyway), but I would guess it's there to make debugging / working on cache related things easier?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good point. I'm still in favor of killing it if it's causing issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the filename prefix is helpful with debugging cache related things, how about we prefix with just the alphanumeric characters from the filename? That should solve any issues across systems but retain the human friendly context

Copy link
Contributor

Choose a reason for hiding this comment

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

@scotthovestadt you fine with this too? I think it's a good way to handle it

const cachePath = slash(
path.join(
cacheDir,
path.basename(filename, path.extname(filename)) + '_' + cacheKey,
),
path.join(cacheDir, cacheFilenamePrefix + '_' + cacheKey),
);
createDirectory(cacheDir);

Expand Down
29 changes: 29 additions & 0 deletions packages/jest-transform/src/__tests__/script_transformer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ describe('ScriptTransformer', () => {

mockFs = object({
'/fruits/banana.js': ['module.exports = "banana";'].join('\n'),
'/fruits/banana:colon.js': ['module.exports = "bananaColon";'].join('\n'),
'/fruits/grapefruit.js': [
'module.exports = function () { return "grapefruit"; }',
].join('\n'),
Expand Down Expand Up @@ -532,6 +533,34 @@ describe('ScriptTransformer', () => {
expect(writeFileAtomic.sync).toBeCalled();
});

it('reads values from the cache when the file contains colons', () => {
const transformConfig = {
...config,
transform: [['^.+\\.js$', 'test_preprocessor']],
};
let scriptTransformer = new ScriptTransformer(transformConfig);
scriptTransformer.transform('/fruits/banana:colon.js', {});

const cachePath = getCachePath(mockFs, config);
expect(writeFileAtomic.sync).toBeCalled();
expect(writeFileAtomic.sync.mock.calls[0][0]).toBe(cachePath);

// Cache the state in `mockFsCopy`
const mockFsCopy = mockFs;
jest.resetModuleRegistry();
reset();

// Restore the cached fs
mockFs = mockFsCopy;
scriptTransformer = new ScriptTransformer(transformConfig);
scriptTransformer.transform('/fruits/banana:colon.js', {});

expect(fs.readFileSync).toHaveBeenCalledTimes(2);
expect(fs.readFileSync).toBeCalledWith('/fruits/banana:colon.js', 'utf8');
expect(fs.readFileSync).toBeCalledWith(cachePath, 'utf8');
expect(writeFileAtomic.sync).not.toBeCalled();
});

it('does not reuse the in-memory cache between different projects', () => {
const scriptTransformer = new ScriptTransformer({
...config,
Expand Down