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 dependencySatisfies invalidates when installed packages change #913

Merged
merged 10 commits into from Aug 9, 2021

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Jul 29, 2021

dependencySatisfies can become invalid if babel caches the macros plugin + the dependency that is being checked for changes its package version. This adds a cache busting plugin that tracks a hash of the contents of the apps lock file so any changes to dependencies will bust the cache.

Fixes: #906

@thoov thoov added the bug Something isn't working label Jul 29, 2021
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.

This seems good overall, though I did leave a couple notes about sharing / reuse.

I'd also love to see a test here, the reproduction in "real world" was pretty straight forward (running npm test which runs multiple ember-try scenarios back to back in an addon ought to have triggered this).

packages/macros/src/babel-plugin-cache-busting.ts Outdated Show resolved Hide resolved
packages/macros/src/ember-addon-main.ts Outdated Show resolved Hide resolved
packages/macros/src/ember-addon-main.ts Outdated Show resolved Hide resolved
packages/macros/src/ember-addon-main.ts Outdated Show resolved Hide resolved
dependencySatisfies can become invalid if babel caches the macros
plugin + the dependency that is being checked for changes its package
version. This adds a cache busting plugin that tracks a hash of the
contents of the apps lock file so any changes to dependencies will
bust the cache.

Fixes: embroider-build#906
@thoov thoov force-pushed the marco-babel-cache-busting branch from 0b0d836 to 0ea82c1 Compare August 4, 2021 23:54
@thoov
Copy link
Collaborator Author

thoov commented Aug 4, 2021

@rwjblue:

I'd also love to see a test here, the reproduction in "real world" was pretty straight forward (running npm test which runs multiple ember-try scenarios back to back in an addon ought to have triggered this).

Added a test to cover this and verified that removing the cache busting plugin it fails the test

@thoov thoov force-pushed the marco-babel-cache-busting branch from 59c554d to f830fe4 Compare August 6, 2021 19:51
@thoov thoov requested a review from rwjblue August 7, 2021 03:15
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.

Overall 👍, only one minor suggestion RE: babelPlugins.unshift (made suggestion inline).

@rwjblue rwjblue merged commit 1a283b1 into embroider-build:master Aug 9, 2021
@rwjblue rwjblue changed the title Bust babel cache if dependencies change so dependencySatisfies is always valid Ensure dependencySatisfies invalidates when installed packages change Aug 9, 2021
thoov added a commit to thoov/ember-engines that referenced this pull request Aug 9, 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.

@embroider/macros uses previously cached macro condition
5 participants