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

Ensure babel transpilation cache is invalided when changing versions of babel plugins or AST transforms #841

Merged
merged 2 commits into from
Jun 11, 2021

Conversation

stefanpenner
Copy link
Collaborator

@stefanpenner stefanpenner commented Jun 9, 2021

This includes 2 fixes:

  1. Fix Placeholder cache solution when used with requireFile

Ensure we include the package’s version in the configuration, so that the babel-loader can detect module version changes in it’s cache key consideration. Without this, installing a new node_module may not result in babel-loaders cache expiration occurring.

  1. Fix babel plugin version change cache busting.

We now ensure a given plugins node_module version is included (when available) in the babel configuration. Which ensures the cache is correctly evicted. This is accomplished by adding our own noop babel plugin into the plugin configuration, and having it contain the various plugin path / version pairs.

@stefanpenner stefanpenner force-pushed the cache-busting branch 2 times, most recently from a45c8ea to 12ecbc1 Compare June 9, 2021 20:24
@stefanpenner stefanpenner changed the title [Fix Placeholder] Cache busting [Fix] Cache busting Jun 9, 2021
Ensure we include the package’s version in the configuration, so that the babel-loader can detect module version changes in it’s cache key consideration. Without this, installing a new node_module may not result in babel-loaders cache expiration occurring.
@@ -144,6 +145,36 @@ export function excludeDotFiles(files: string[]) {
return files.filter(file => !file.startsWith('.') && !file.includes('/.'));
}

export const CACHE_BUSTING_PLUGIN = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this fixes (2)

useMethod?: string;
}

const { findUpPackagePath } = resolvePackagePath;

export function maybeNodeModuleVersion(path: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixes 1

@stefanpenner stefanpenner force-pushed the cache-busting branch 2 times, most recently from 9295005 to 3eb7828 Compare June 10, 2021 19:53
packages/core/src/portable.ts Outdated Show resolved Hide resolved
packages/core/src/portable.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Is it possible to write a test for the cache invalidation issue that this resolves?

@stefanpenner stefanpenner force-pushed the cache-busting branch 3 times, most recently from 6dddcda to fad8137 Compare June 10, 2021 22:46
@stefanpenner
Copy link
Collaborator Author

Is it possible to write a test for the cache invalidation issue that this resolves?

We could add another CI worker to run sequential ember-try scenarios... but it's unclear we can just continue to add those CI jobs... if we want to do that sure, otherwise I think this is adequate given our current priorities.

@stefanpenner stefanpenner requested a review from rwjblue June 10, 2021 23:36
@stefanpenner
Copy link
Collaborator Author

@ef4 / @rwjblue should we also include the current node version / OS in the cache key? ... If needed we can always expand later...

@rwjblue
Copy link
Collaborator

rwjblue commented Jun 11, 2021

should we also include the current node version / OS in the cache key?

Probably. I think we should likely also include targets info, though it may already be included (since its probably part of options to @babel/preset-env's preset).

@stefanpenner
Copy link
Collaborator Author

Probably. I think we should likely also include targets info, though it may already be included (since its probably part of options to @babel/preset-env's preset).

I believe these are in there already

@stefanpenner
Copy link
Collaborator Author

@rwjblue with a fresh mind tomorrow I’ll think about approaches to do a more e2e test that is the right cost/benefit.

packages/core/src/app.ts Outdated Show resolved Hide resolved
packages/core/src/portable.ts Show resolved Hide resolved
Today, babel’s caches (via babel-loader today) expires caches only if
the options passed to it change, unfortunately if you upgrade a plugin
without changing the configuration options and the file contents remains
the same, the cache will not be evicted. This can lead to spurious
caching problems if the associated plugins behavior has changed.

To address this defect we now include (when available) the
package.json#version of the babel plugins used in our own no-op babel
plugin’s configuration. This new no-op plugin is then inserted at the
end of the configuration in question.
@rwjblue rwjblue merged commit 9426ad8 into master Jun 11, 2021
@rwjblue rwjblue deleted the cache-busting branch June 11, 2021 20:01
@rwjblue rwjblue changed the title [Fix] Cache busting Ensure cached template compiler results are invalidated when the template compiler changes (e.g. across Ember versions) Jun 15, 2021
@rwjblue rwjblue added the bug Something isn't working label Jun 15, 2021
@rwjblue rwjblue changed the title Ensure cached template compiler results are invalidated when the template compiler changes (e.g. across Ember versions) Ensure babel transpilation cache is invalided when changing versions of babel plugins or AST transforms Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants