tildeio / ember-element-helper

Dynamic element helper for Glimmer templates.
MIT License
44 stars 23 forks source link

Loosen `peerDependencies` requirement for `ember-source` #80

Closed SergeAstapov closed 2 years ago

SergeAstapov commented 2 years ago

Using peerDependencies with specific versions range may cause issues in monorepo setup when ember-source versions don't match and the wrong one may be hoisted to the top.

This caused problems when tried to port ember-animated to v2 in https://github.com/ember-animation/ember-animated/pull/388: when running ember-try 3.20 ember-try scenario, yarn/npm as well as pnpm hoisted ember-source@4 to the top level and as a result @emberoider/utils detect v4.1 when the test-app actually used 3.20.

As far as I understand, there is no benefit having specific version(s) listed in peerDependencies as it may force package manager install extra ember-source in addition to ember-source used by host app and it's not needed for this addon to function.

SergeAstapov commented 2 years ago

@cibernox @simonihmig mind to approve CI and take a look into this?

simonihmig commented 2 years ago

I approved CI, however I am not sure about the changes here. Isn't * *effectively the same as what we had before? How does it make a difference?

As far as I understand, there is no benefit having specific version(s) listed in peerDependencies as it may force package manager install extra ember-source in addition to ember-source used by host app

That is not my understanding of peerDependencies, but I may be wrong. I thought it means the host must provide the dependency, and if it is not, that is an error, but the package manager will not install it on its own?

SergeAstapov commented 2 years ago

Isn't * *effectively the same as what we had before? How does it make a difference?

By "before" do you mean v0.5.5 of this addon? It did not have any peerDependencies listed. So technically answer is yes.

Looks like the reason peerDependencies entry was added in this PR - because dependencySatisfies being used in tests (and for tests we don't need to list ember-source in peerDependencies)

the host must provide the dependency, and if it is not, that is an error, but the package manager will not install it on its own

unfortunately this depends on package manager (and version), e.g. npm 6 did not install peer deps whereas npm 7 does install peerDependencies per https://github.com/npm/rfcs/blob/main/implemented/0025-install-peer-deps.md yarn usually complains about missing peer dependencies.

I can workaround this issue in ember-animated itself, however I don't see reason why ember-element-helper would need to have specific ember versions listed in it's peerDependencies (unless I'm missing something).

SergeAstapov commented 2 years ago

We were able to find a workaround for this problem by using pnpm with dependenciesMeta.*.injected option:

in ember-animated/test-app/package.json added such config:

"dependenciesMeta": {
  "ember-animated": {
    "injected": true
  }
}

This ensures proper peer dependencies resolution in the test app.

The downsides of this approach can be found in this discussion.