search

embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
333 stars 136 forks source link

Feature idea: isActiveAddon macro #1956

Open simonihmig opened 3 months ago

simonihmig commented 3 months ago

I have been trying to implement support for optional peer dependencies using a pattern like

if (macroCondition(dependencySatisfies('fantastic-but-optional-package', '*'))) {
  // use the optional stuff here
}

The problem here is when using good (bad) old yarn v1 in a monorepo, which is hoisting all dependencies. dependencySatisfies basically answers the question "is this package resolvable?" (and does it match the version), but with hoisting in place the answer to this is always "yes" (when some other workspace package brings in that dependency, but not the current app). And yes, we want to get rid of yarn v1, but that's not a short-term possibility...

I was thinking if it would make sense to provide a new macro like isActiveAddon, which is not looking for resolvability, but looking into the app's "active addons" (that EmberCLI is tracking already, by traversing the dependency graph of addons and their dependencies, but ignoring peer deps).

This might sound a bit like a step backwards into Ember's legacy world, on the other hand we are coming from this world, and having this could be a stop gap solution to adopt better patterns even when we are not fully there yet in the new world...

FWIW, with the pattern above, and no other app (or addon) fulfilling that optional peer dependency, while it was still resolvable, I was getting this exception thrown in Embroider builds: 

bug in @embroider/core/src/module-resolver: cannot figure out the owning engine for /path/to/fantastic-but-optional-package

The place where this is happening is looking into activeAddons, so first we have that concept already, and second I believe if I was able to prevent the code using the optional addon to be around when the addon is resolvable but not active, then this would not have occurred.

ef4 commented 3 months ago

Point of clarification: dependencySatisfies is already supposed to check for an explicit dependency, not just accidental resolvability:

https://github.com/embroider-build/embroider/blob/4d74274fba558196c5c82f322257b09adfd1f0e3/packages/macros/src/babel/dependency-satisfies.ts#L27-L29

simonihmig commented 3 months ago

Yes, I know.

But I think it is the optional peer dependency case that it is getting "wrong" here. If package A has an optional peer dep on B, this makes us?.hasDependency('B') return true, because it only looks into the peerDependencies key here, without making a difference about normal vs. optional peer deps, right? So in the case of an optional one, if the app does not fulfil that peer dep (which it may choose to not do), then I would like the macroCondition to evaluate to false, but the way dependencySatisfies works it will return true. (again, only with hosted dependencies)