Refuse to accept v1 addons as invalid peerDeps #1542

ef4 · 2023-07-17T16:11:16Z

The biggest source of our infamous empty package output issue is when package managers give us fundamentally broken graphs of peer dependencies.

We have spent a lot of effort debugging people's build issues only to discover that their node_modules have fundamentally wrong peer dependencies. So this change refuses to build when a peer dependency that is a v1 addon resolves incorrectly.

It's limited to v1 addons, not all packages, because many things in the ecosystem do in fact keep working with bad peer dependencies. But for us to correctly convert a v1 addon to a v2 addon, we need to see it getting consumed (within the classic ember-cli Addon / App / Project infrastructure) by some other package in the build. And these bad peers do not get reliably consumed, resulting in empty output and confusing failures.

Even if we added hacks to try to make them work, it would result in unintentional duplication and bloat within the app. So I'm just going to blow up the build. People need to use overrides or hoisting to collapse down these bad dependency graphs.

I left an escape hatch via process.env.I_HAVE_BAD_PEER_DEPS_AND_WANT_A_BROKEN_BUILD. Users of such had better not report bugs to me. 😂 But it may be useful if you're in the middle of debugging a problem and need to see what happens after this point.

This PR also cleans up some code in buildCompatAddon that is no longer needed since the change that eliminated total node_modules rewriting. V2 addons don't need to ever pass through this spot anymore.

The biggest source of our infamous [empty package output](https://github.com/embroider-build/embroider/blob/main/docs/empty-package-output.md) issue is when package managers give us fundamentally broken graphs of peer dependencies. We have spent a lot of effort debugging people's build issues only to discover that their node_modules have fundamentally wrong peer dependencies. So this change refuses to build when a peer dependency that is a v1 addon resolves incorrectly. It's limited to v1 addons, not all packages, because many things in the ecosystem do in fact keep working with bad peer dependencies. But for us to correctly convert a v1 addon to a v2 addon, we need to see it getting consumed (within the classic ember-cli Addon / App / Project infrastructure) by some other package in the build. And these bad peers do not get reliably consumed, resulting in empty output and confusing failures. Even if we added hacks to try to make them work, it would result in unintentional duplication and bloat within the app. So I'm just going to blow up the build. People need to use overrides or hoisting to collapse down these bad dependency graphs. I left an escape hatch via `process.env.I_HAVE_BAD_PEER_DEPS_AND_WANT_A_BROKEN_BUILD`. Users of such had better not report bugs to me. 😂 But it may be useful if you're in the middle of debugging a problem and need to see what happens after this point. This PR also cleans up some code in buildCompatAddon that is no longer needed since the change that eliminated total node_modules rewriting. V2 addons don't need to ever pass through this spot anymore.

NullVoxPopuli · 2023-07-17T16:26:04Z

packages/compat/src/standalone-addon-build.ts

+  let violations = validatePeerDependencies(appPackage).filter(({ dep }) => dep.isEmberPackage() && !dep.isV2Ember());
+  if (violations.length > 0) {
+    if (process.env.I_HAVE_BAD_PEER_DEPS_AND_WANT_A_BROKEN_BUILD) {
+      console.warn(


🎉 I love it

NullVoxPopuli · 2023-07-17T16:28:11Z

packages/shared-internals/src/dep-validation.ts

+  let queue: { pkg: Package; path: Package[] }[] = [{ pkg: startingPackage, path: [] }];
+  let seen = new Set<Package>();
+  let results = new Map<Package, Package[][]>();
+  for (;;) {


unrelated to the goal of the Pr, but moreso a curiosity,
I suspect this is equiv to while (true) -- and in any case is used to to iterate over queue, but safely as its contents are changing during iteration.

any reason to favor one method of implicitly-bounded looping over another?

They're equivalent. I got into the habit of using this one because I've encountered lint rules that forbid while (true) (which is itself dumb, but 🤷‍♂️)

NullVoxPopuli · 2023-07-17T16:31:33Z

packages/shared-internals/src/dep-validation.ts

+  ancestorsDep: Package;
+}
+
+export function validatePeerDependencies(appPackage: Package): PeerDepViolation[] {


is this pretty fast?

academically, it's O(N^4), but practically, are the iterations of the nested loops even that numerous? (like, ancestors and consumptions are both probably pretty small, yeah?)

It's not really N^4 (where N is the number of packages), because those are different N's at the different levels. The outermost level is O(N) where N=number of packages. But the next level is O(P) where P = average number of peer deps a package has. Which is probably closer to zero than one. That immediately chops off a vast swath of the space.

Then if you do have peer deps, the next level is the number of times your package is consumed, and the final level is the number of level we have to look upward to find the provider of the peer dep.

It's quite fast, and it's all traversing our in-memory package cache.

void-mAlex

@ef4 when you talk about consumption of peer deps, does that also consider an addon that would only expose a service - the error reporting type of addon or is that something unrelated?
I would think we need some changes in the stage3 to check this

NullVoxPopuli · 2023-07-17T17:22:28Z

given the new errors in Ci,
ember-resolver 10.1.1 widens the peer range to include ember-source@v5 if that helps

ef4 · 2023-07-17T17:41:28Z

I enjoy that this found a place in our own monorepo where three different copies of ember-source, ranging from 3.x to 5.x, were visible within one application.🙃

ef4 · 2023-07-17T17:44:26Z

@ef4 when you talk about consumption of peer deps, does that also consider an addon that would only expose a service - the error reporting type of addon or is that something unrelated?
I would think we need some changes in the stage3 to check this

I'm not exactly sure if I understand your question but I think it's unrelated.

This is purely about checking that if somebody declares a peer dep on an addon, they better actually get that peer dep satisfied correctly, such that they really see the same copy as their consumers see.

NullVoxPopuli · 2023-08-20T12:17:44Z

I've noticed that it's hard / impossible to link to another project outside my repo. (pnpm link)

I thought that maybe using depMeta*injected could help, but no dice. 🤔
Not sure what to do about this yet, but wanted to report here for SEO.

So far, I've tried:

declare package as depMeta*injected
link the package
in the package, run pnpm i --prod, to remove devDeps

pnpm start gives:

stack: TypeError: Cannot read properties of undefined (reading 'name')
at packageIdSummary (/home/nvp/Development/NullVoxPopuli/limber/node_modules/.pnpm/@[email protected].
  0/node_modules/@embroider/shared-internals/src/dep-validation.js:91:19)

same outcome after syncing injected deps
It looks like this error occurs when it's trying to print the peer / duplicates messaging, but the dep is undefined

So, trying a new approach, I decided to give the file: protocol a go, and it works.
So, I guess that's the work-around for how -- I would def consider this a pnpm bug tho

NullVoxPopuli reviewed Jul 17, 2023

View reviewed changes

NullVoxPopuli approved these changes Jul 17, 2023

View reviewed changes

NullVoxPopuli reviewed Jul 17, 2023

View reviewed changes

void-mAlex reviewed Jul 17, 2023

View reviewed changes

dropping unused conflicting dependency

01feb85

ef4 merged commit 812df54 into main Jul 17, 2023
196 checks passed

ef4 deleted the peer-deps-check branch July 17, 2023 22:48

ef4 added the bug Something isn't working label Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refuse to accept v1 addons as invalid peerDeps #1542

Refuse to accept v1 addons as invalid peerDeps #1542

ef4 commented Jul 17, 2023

NullVoxPopuli Jul 17, 2023

NullVoxPopuli Jul 17, 2023

ef4 Jul 17, 2023

NullVoxPopuli Jul 17, 2023

ef4 Jul 17, 2023

void-mAlex left a comment

NullVoxPopuli commented Jul 17, 2023

ef4 commented Jul 17, 2023

ef4 commented Jul 17, 2023

NullVoxPopuli commented Aug 20, 2023 •

edited

Loading

Refuse to accept v1 addons as invalid peerDeps #1542

Refuse to accept v1 addons as invalid peerDeps #1542

Conversation

ef4 commented Jul 17, 2023

NullVoxPopuli Jul 17, 2023

Choose a reason for hiding this comment

NullVoxPopuli Jul 17, 2023

Choose a reason for hiding this comment

ef4 Jul 17, 2023

Choose a reason for hiding this comment

NullVoxPopuli Jul 17, 2023

Choose a reason for hiding this comment

ef4 Jul 17, 2023

Choose a reason for hiding this comment

void-mAlex left a comment

Choose a reason for hiding this comment

NullVoxPopuli commented Jul 17, 2023

ef4 commented Jul 17, 2023

ef4 commented Jul 17, 2023

NullVoxPopuli commented Aug 20, 2023 • edited Loading

NullVoxPopuli commented Aug 20, 2023 •

edited

Loading