Ensure dependencySatisfies only considers actual dependencies (includes a fix for invalid results within monorepo scenarios)
#1070
Conversation
| @@ -12,7 +12,7 @@ export default function dependencySatisfies(path: NodePath<t.CallExpression>, st | |||
| if (path.node.arguments.length !== 2) { | |||
| throw error(path, `dependencySatisfies takes exactly two arguments, you passed ${path.node.arguments.length}`); | |||
| } | |||
| let [packageName, range] = path.node.arguments; | |||
| const [packageName, range] = path.node.arguments; | |||
There was a problem hiding this comment.
needed to be const due to the function usage in line 35
|
can I make this draft after submission? 🙃
|
|
@NullVoxPopuli I converted it to draft |
|
@lifeart thank you! |
|
Some of the existing babel tests were misleading you I think ( There is a similar step still needed for the glimmer tests I think. |
|
The remaining test failures happen because the PackageCache at this layer doesn't distinguish which Package is the app vs any other library, and I'm refactoring PackageCache so that threading through the app root path is mandatory, so it can always give correct answers about the app's dependencies. Up until now we hadn't needed that here. |
|
Thanks for poking at this! This'll be a huge help! |
|
The two remaining failures are just sporadic, I confirmed they're passing locally. (We have so many different jobs that even low-probability failures in CI are likely to accumulate and cause a bad run.) |
|
This is ready to go. I hijacked your PR and made it a much better change. 😄 One thing we can consider doing next is making packageCache.resolve strict like this by default. There are definitely some cases in embroider/compat where we'd want to opt into non-strict resolve, but it would be a good default. |
|
Thank you!! <3 |
rwjblue
changed the title
dependencySatisfies only considers actual dependencies (includes a fix for invalid results within monorepo scenarios)
This addon uses `dependencySatisfies` to check the ember-source version. But unless you have a peerDep on ember-source, that was unreliable depending on Yarn/NPM optimization behaviors. And now as of embroider-build/embroider#1070 we're making `dependencySatisfies` strict so that it won't accidentally work sometimes when you don't actually declare a dependency or peerDependency.
This package uses `dependencySatisfies` of `@embroider/macros` which is only reliable if a `peerDependency` for the package exists. If not, the macro checks for a dependency in the `ember-engines` package when a monorepo structure is used. This behaviour happens since embroider v0.50.1 which includes an important fix for monorepos: embroider-build/embroider#1070
|
Thanks for this fix, this resolves a lot of issues I had with packages using Also, I'm wondering if a peer dependency should really be needed - what happens if a package wants to check whether a certain package is installed or not? A peer dependency for such a case would be wrong, no? This might be what @ef4 meant here: #1070 (comment) ? |
This package uses `dependencySatisfies` of `@embroider/macros` which is only reliable if a `peerDependency` for the package exists. If not, the macro checks for a dependency in the `ember-engines` package when a monorepo structure is used. This behaviour happens since embroider v0.50.1 which includes an important fix for monorepos: embroider-build/embroider#1070
For that we use optional peer dependencies. |
This package uses `dependencySatisfies` of `@embroider/macros` which is only reliable if a `peerDependency` for the package exists. If not, the macro checks for a dependency in the `ember-engines` package when a monorepo structure is used. This behaviour happens since embroider v0.50.1 which includes an important fix for monorepos: embroider-build/embroider#1070
Resolves: #1067