search

ember-graphql / ember-apollo-client

🚀 An ember-cli addon for Apollo Client and GraphQL
MIT License
279 stars 72 forks source link

Remove global Ember import #413

Closed balinterdi closed 2 years ago

balinterdi commented 2 years ago

This is one of the deprecations that prevent Ember 4 compatibility.

isTesting can be imported from @embroider/macros and relying on this package does not mean the add-on needs to be built with Embroider.

balinterdi commented 2 years ago

@josemarluedke I wouldn't have merged this as it still fails on ember-canary (and maybe on other Ember versions, too, but they were cancelled early) 😬

josemarluedke commented 2 years ago

I believe it's failing because ember-auto-import v1 is used and ember v4 requires v2.

balinterdi commented 2 years ago

If that was the case then #414 would pass, wouldn't it? 🤔

josemarluedke commented 2 years ago

Yes, when rebased with the changes from this PR.

josemarluedke commented 2 years ago

Published as v4.0.0.

bertdeblock commented 2 years ago

I think the PR description is a bit confusing. import Ember from 'ember'; was never deprecated, and the removed code works just fine in v4. We have been using ember-apollo-client in v4 for a while now. That being said, using macroCondition(isTesting()) is probably better anyways to strip out testing code.

balinterdi commented 2 years ago

@bertdeblock You're right. I was under the impression that importing the global Ember will throw an error in Ember 4 but now I re-read the entry in the deprecation guide and it turns out that it only throws if you use Ember without importing it.

I still think this is very valuable to have (otherwise this add-on prevents tree-shaking parts of Ember) but the description is indeed misleading, @josemarluedke sorry about that.

bertdeblock commented 2 years ago

I still think this is very valuable to have

Yup, this seems better anyways 👍