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

Fix cacheKeyForTree & OneShot incompatibility #1100

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Feb 1, 2022

The fix in #1064 turns out to be insufficient.

The problem is that cacheKeyForTree can provide a cached tree at any point in the broccoli graph. We can't tell just by looking at the top level node for an addon whether it contains, deep down inside it, some trees that have already been run by other builders via OneShot.

I don't like cacheKeyForTree. It's trying to solve a real problem, but at the wrong layer. Instead of sharing work between many addon instances, it should have been preventing there from being many addon instances in the first place.

This PR moves Embroider to a solution more like that. We already have the reduceInstances hook. Here I give it a default implementation that will allow two addon instances with identical cacheKeyForTree for all trees to deduplicate at the instance level, not the tree level.

It's also important to point out that there are up to three different kinds of deduplication going on in a classic build via cacheKeyForTree:

  • deduplication between multiple instances of the same copy of a package
  • deduplication between different copies (which may or may not have the same version) of the same package
  • deduplication between entirely unrelated packages

Our reduceInstances hook intentionally only covers the first case.

For the second case (when you want to deduplicate different copies of a package) you can do that in your package manager instead via yarn resolutions or NPM overrides.

For the third case (when you want to deduplicate between entirely unrelated packages)... no. Just don't. That is the worst kind of hidden global monkey patching.

The fix in #1064 turns out to be insufficient.

The problem is that `cacheKeyForTree` can provide a cached tree at any point in the broccoli graph. We can't tell just by looking at the top level node for an addon whether it contains, deep down inside it, some trees that have already been run by other builders via OneShot.

I don't like `cacheKeyForTree`. It's trying to solve a real problem, but at the wrong layer. Instead of sharing work between many addon instances, it should have been preventing there from *being* many addon instances in the first place.

I'm moving Embroider to a solution more like that. We already have the `reduceInstances` hook, which is a more blunt and powerful way to through away whole Addon instances. We can give it a default implementation that will allow two addons with *entirely* identical cacheKeyForTree to deduplicate at the *instance* level, not the tree level.
@ef4 ef4 merged commit e69746b into main Feb 2, 2022
@ef4 ef4 deleted the cachekeyfortree-fix branch February 2, 2022 02:56
@rwjblue rwjblue added the bug Something isn't working label Feb 7, 2022
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