Remove 'peerDependencies' from 'dependencyKeys' to align with ember-cli

[charlespierce]

Info

• Currently, Embroider will include an addon listed in peerDependencies as part of the app-js when merging lazy engines.
• This doesn't match with ember-cli, which completely ignores peerDependencies.
• It can lead to including the wrong files in a lazy engine, when its addons only have peerDependencies on something that is included by the root app.

Changes

• Added addonDependencies getter to Package, mirroring what dependencies does but excluding those dependencies in peerDependencies
• Included addonDependencies in the MovedPackageProxy so that moved packages will be handled correctly as well.
• Updated activeAddonChildren to crawl the addonDependencies instead of the full dependencies list to get the child addons.

Notes

• It looks like there are some unrelated ember-canary tests failing due to new deprecation warnings.

[ef4]

This is just how NPM packages are supposed to behave. And v2 addons definitely need it because there's no other way to resolve something out of the app.

If we want classic addons to not do this, we should delete their peerDependencies while we're rewriting them from v1 to v2.

[charlespierce]

Yeah, it's possible we need to tackle this from a different angle. The main issue isn't the existence of peerDependencies, rather the loading of those peer dependencies as child addons into lazy engines (which then results in duplication of assets). I'm happy to update this, however I'm not 100% where to look in the code for that behavior.

[charlespierce]

Ah ha, I think I see where the change needs to happen (though I don't 100% see how to do it just yet): The partitionEngines method within App is using the Package resolution to determine which of the full list of addons should be included. However, as noted above, this differs from the classic build process since that only uses the dependencies, not peerDependencies. So I think the necessary change is actually to update partitionEngines to properly detect each engine's children, rather than cutting off the peerDependencies entirely.

[ef4]

That sounds promising. 👍

The other place I was mentioning is here, where we already delete some things from package.json. But if you only need to change the engine splitting behavior, that's even better

[charlespierce]

Updated with a new approach: limiting it to only excluding peerDependencies from the partitionEngines rollup. However, thinking on it some more, isn't it also the case that:

• If a peerDependency is resolvable in node_modules, then by definition some other project—typically the parent app—has it as a dependency
• We will always crawl that the dependency (and move it / symlink as necessary) as part of crawling whichever package lists it as an "actual" dependency (as opposed to only a peer dependency).
• Therefore, we don't need to crawl peerDependencies at all, because addons there are always excluded and any other package there will get handled appropriately by whatever package declares them in dependencies.

I suspect there's a corner case that I'm missing with that logic, but it seems sound on the surface. If that's true, then it may be cleaner to simply skip over them entirely.

[ef4]

@charlespierce I suggested a change in charlespierce#1. Can you can confirm that it still provides the desired performance benefit in your app?

[charlespierce]

Hi @ef4 yep, confirmed that solves the problem as well and merged it into the PR branch. My only concern is if an addon has a dependency listed in devDependencies and peerDependencies, then a local build would pick that up under classic but not under embroider.

However, since a consuming app would then lose that addon dependency (even if it was installed separately, because the classic build would ignore both devDeps and peerDeps), so that seems like an anti-pattern anyway.

[ef4]

Yeah, I sat and pondered that case myself and decided I could live with it. 😄

Thanks!