Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitive Dependency Resolution Breaks Build on Recursive v2 Addons #591

Closed
gossi opened this issue Aug 19, 2023 · 2 comments
Closed

Transitive Dependency Resolution Breaks Build on Recursive v2 Addons #591

gossi opened this issue Aug 19, 2023 · 2 comments

Comments

@gossi
Copy link

gossi commented Aug 19, 2023

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:

CallExpression(path: NodePath<t.CallExpression>, state: State) {
if (state.handled.has(path.node)) {
// We see the same CallExpression multiple times if it has a
// TemplateLiteral argument and another plugin or preset is rewriting
// TemplateLiterals to something else. We need to guard against that
// because the first time is fine and we capture our analysis, but the
// second time would cause us to throw an exception when we see an
// illegal argument.
return;
}
let callee = path.get('callee');
if (callee.type === 'Import') {
state.imports.push(processImportCallExpression(path, true));
state.handled.add(path.node);
} else if (
callee.isIdentifier() &&
callee.referencesImport('@embroider/macros', 'importSync')
) {
state.imports.push(processImportCallExpression(path, false));
state.handled.add(path.node);
}
},

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
Copy link
Contributor

NullVoxPopuli commented Aug 19, 2023

@gossi
Copy link
Author

gossi commented Aug 19, 2023

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.

@gossi gossi closed this as completed Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants