Transitive Dependency Resolution Breaks Build on Recursive v2 Addons

gossi commented 10 months ago

I'm working on ember-ability which has a peerDependency to ember-resources. In my test-matrix the tests fail, because @ember/owner cannot be found for ember versions < 4.12. The PR in question: gossi/ember-ability#12

In more detailed steps:

The little code for ember-ability is a fairly tiny, 27loc.

The complete error message:

 not ok 1 Chrome 115.0 - [2 ms] - TestLoader Failures: test-app/tests/unit/ability-test: could not be loaded
    ---
        actual: >
            null
        stack: >
            Error: Could not find module `@ember/owner` imported from `ember-ability`
                at missingModule (http://localhost:7357/assets/vendor.js:259:11)
                at findModule (http://localhost:7357/assets/vendor.js:270:7)
                at Module.findDeps (http://localhost:7357/assets/vendor.js:180:24)
                at findModule (http://localhost:7357/assets/vendor.js:274:11)
                at Module.findDeps (http://localhost:7357/assets/vendor.js:180:24)
                at findModule (http://localhost:7357/assets/vendor.js:274:11)
                at requireModule (http://localhost:7357/assets/vendor.js:36:15)
                at TestLoader.require (http://localhost:7357/assets/test-support.js:7817:9)
                at TestLoader.loadModules (http://localhost:7357/assets/test-support.js:7811:14)
                at loadTests (http://localhost:7357/assets/test-support.js:8308:22)

Which kept me wondering, since ember-ability has no direct dependency to @ember/owner. I traced it down and apparently ember-resources does have a dependency to @ember/owner in: https://github.com/NullVoxPopuli/ember-resources/blob/c4948618dfc94b81ed4c68dcbf27e322d0371d2c/ember-resources/src/core/function-based/immediate-invocation.ts#L24-L32 but this is behind a condition using macros:

if (macroCondition(dependencySatisfies('ember-source', '>=4.12.0'))) {
  // In no version of ember where `@ember/owner` tried to be imported did it exist
  // if (macroCondition(false)) {
  // Using 'any' here because importSync can't lookup types correctly
  setOwner = (importSync('@ember/owner') as any).setOwner;
} else {
  // Using 'any' here because importSync can't lookup types correctly
  setOwner = (importSync('@ember/application') as any).setOwner;
}

I did enable forbidEval: true in ember-auto-import for leaner output and found this:

d('ember-ability', ['@glimmer/tracking/primitives/cache','@ember/destroyable','@ember/helper','@ember/application','@ember/debug','@ember/owner','@glimmer/tracking'], function() { return __webpack_require__(/*! ember-ability */ "../ember-ability/dist/index.js"); });

which later on is transformed into a require() statement (as far as understood) - yet none of the dependencies is actually used in the function() call.

Since, these are none of the dependencies that ember-ability uses directly, those are the transitive dependencies, too. This brings me back to the code from ember-resources above, which has an importSync() to both @ember/owner and @ember/application, which is why I think BOTH OF THEM are in the list of deps.

If I'm not mistaken, this code finds them: https://github.com/embroider-build/ember-auto-import/blob/843ed86e7fc61c5c29c012738b905a551cb4d49a/packages/ember-auto-import/ts/analyzer-plugin.ts#L46-L67

Thus the analyzer finds imports based on symbols, but is not evaluating the code here. The macroCondition(dependencySatisfies('ember-source', '>=4.12.0')) expression is not evaluated and such a dependency, in this case @ember/owner, is added as dependency, which in reality isn't.

Which I think is the root cause for why my build is failing.

NullVoxPopuli commented 10 months ago

For exploratory reasons,

Does this all get resolved if you change your test-app's package.json to specify:

  "dependenciesMeta": {
    "ember-ability": {
      "injected": true
    }
  },

and then pnpm build, pnpm i -f?

Potentially related:

gossi commented 10 months ago

Yeah, that does 🙈

Using your pnpm-sync-dependencies-meta-injected in the test-app does the job. I gonna re-trigger CI, then we should see it.

That solved all of the CI problems 😀 Thank you very much. Can close this issue.

embroider-build / ember-auto-import

Transitive Dependency Resolution Breaks Build on Recursive v2 Addons #591