dependencySatisfies("ember-source", ...) is "broken"

[chancancode]

It seems like #1070 broke all the dependencySatisfies("ember-source", ...) in the wild. Just wanted to confirm this is intentional (for "ember-source" specifically).

"ember-source" is a defacto peer dependency for all Ember add-ons for somewhat obvious reasons. It maybe fine to start encouraging add-ons to explicitly add that peer dependency going forward, though I don't think that's been discussed much and I vaguely remember there are some problems associated with that (?).

In any case, almost none of the add-ons do this today, so if this is something we want we should add it to the addon blueprint. In the meantime, should we (I think we should?) special case this and treat "keywords": ["ember-addon"] to be the same as "peerDependencies": { "ember-source": "*" }?

[ef4]

I did consider this, but decided it's a bug fix. Addons that try to use dependencySatisfies without a peer dependency were already unreliable -- this makes their failure predictable instead of sporadic.

dependencySatisfies follows real node_modules resolution rules. Without a peerDependency, there is no guarantee that the package manger will make ember-source resolvable from the addon. This was seen most often in monorepo situations, but it would also break any attempts to use more strictly-correct package managers like Yarn>=2 or pnpm.

The embroider RFC says explicitly that dependencySatisfies only resolves "allowed dependencies" and defines "allowed dependencies" of an addon to be only dependencies and peerDependencies, so this was the original intended behavior.

I agree that the whole package management space with peerDependencies is painful. NPM and yarn both get peerDeps wrong in several situations. But we are committed to following node_modules resolution mostly because of typescript -- it cannot easily be taught a different resolution strategy.

And we're committed to making dependencySatisfies follow the same resolution rules as general module imports. You don't want to get inconsistent results between dependencySatisfies and import() or importSync(). They should see the same exact packages.

[chancancode]

Just to be clear, I am specifically referring to ember-source in ember addons only, but if that’s been considered already then it’s all good (we should update the blueprint though.

[mansona]

So alas I don't think it's as simple as you're saying @ef4 😞 the example that I've come across is this one: https://github.com/embroider-build/embroider/blob/main/packages/util/addon/ember-private-api.js#L9-L19 and it caused our app to explode (in dev) because we're using deprecation-workflow and we have global Ember access configured to throw.

I'm not entirely sure what is making use of the ember-private-api because I don't fully understand the callstack 🤔 but in this case there is a peer dependency on ember-source from @embroider/util

[chancancode]

@mansona what is the ember-source version and @embroider/* versions in the app, and is this in a monorepo? Are you able to reproduce it in a standalone app by any chance? I don't think I have seen the case where there is a peerDep and still failing the dependencySatisfies check yet so far.

[ef4]

@mansona I can almost guarantee this is yarn or NPM giving you an incorrect node_modules that doesn't respect peerDependencies. For monorepos, we have had success switching to pnpm because it can be told to do them correctly (using dependenciesMeta.*.injected).

[ef4]

You can try running npx are-my-node-modules-messed-up.

[patricklx]

I also had the same issue because of using different versions of core, utils etc. Especially because i had an overwrite/resolution on core to fix an issue with babel. Setting the Overwrite/resolution for others also to 1.6.0 fixed my issues

[erikkessler1]

I'm running into this as well with a monorepo that has workspaces with different versions of ember-source. Should this line:

embroider/packages/macros/src/babel/dependency-satisfies.ts

Line 26 in aec11b7

let us = state.packageCache.ownerOfFile(state.sourceFile);

be changed to the following since we want to be evaluating the dependencies as the appRoot?

let us = state.packageCache.ownerOfFile(state.packageCache.appRoot);

Making this change fixes things for me, but I'm not sure what the original reason for using sourceFile was.

[ef4]

since we want to be evaluating the dependencies as the appRoot?

No, it's quite important that it's checking relative to the file that is asking.

The bottom line here is that your monorepo is really truly giving the file in question access to the wrong copy of ember-source. dependencySatisfies is just faithfully pointing that out. Your monorepo setup (like all yarn 1 and NPM monorepo setups that involve peer dependencies) is subtly broken, and that is going to cause trouble for you down the line, particularly as we start consuming Ember itself as ES modules.

[erikkessler1]

Your monorepo setup (like all yarn 1 and NPM monorepo setups that involve peer dependencies) is subtly broken

Can you explain more about what is broken about it? Is this an issue with Yarn 1 itself or how I have it configured?

[ef4]

With yarn itself. And NPM. The only client I've seen that gets it correct currently in pnpm.

The problem is this: assume some-addon has a peerDependency on ember-source. And assume two apps in a monorepo have different versions of ember-source. Yarn and NPM will typically do something like this:

.
├── node_modules
│   ├── ember-source
│   └── some-addon
└── workspaces
    ├── appOne
    └── appTwo
        └── node_modules
            └── ember-source

appOne is resolving the copy of ember-source in node_modules/ember-source. appTwo is resolving the copy in workspaces/appTwo/node_modules/ember-source. Both apps are using some-addon and resolve the only copy.

A peer dependency is supposed to mean that some-addon resolves its parent's copy of ember-source. But in this setup, it actually always resolves appOne's copy, even when some-addon is the child of appTwo.

You can work around these problems in yarn 1 by using the nohoist option to keep things more separate.

The above assumes that some-addon is a package consumed from NPM. If instead it's another workspace in the monorepo, you can't work around it with nohoist. To get that case right you can use pnpm with the dependenciesMeta.*.injected option option.

[erikkessler1]

Thank you for the detailed explanation, that makes sense. I was able to get things working by nohoist-ing a bunch of Ember-related packages. For anyone following that is curious:

{
  "nohoist": [
      "appOne/@embroider/*",
      "appOne/@ember/*",
      "appOne/@glimmer/*",
      "appOne/ember-*"
    ]
}