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

Remove implicit injection #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gilest
Copy link

@gilest gilest commented Nov 23, 2022

  • Didn't manage to run the test suite locally using node 16 or node 12 🤷🏻
  • Couldn't find any reference of the storefront service in the docs. Presumably users running Ember 4 are familiar with the need to explicitly inject services, anyway

Fixes #485

@gilest gilest changed the title chore: remove implicit injection Remove implicit injection Nov 23, 2022
@gilest
Copy link
Author

gilest commented Dec 5, 2022

@ryanto any chance we could get a merge and release of this? 🙇🏻

@ryanto
Copy link
Member

ryanto commented Dec 17, 2022

Hey @gilest! Thanks so much for taking the time to put together this PR! I really appreciate it.

Anyway we can get the CI tests passing? If you need to remove older versions that are no longer LTS thats perfectly fine.

Also, this feature is specific to Ember 4.x?

@gilest
Copy link
Author

gilest commented Dec 17, 2022

Anyway we can get the CI tests passing? If you need to remove older versions that are no longer LTS thats perfectly fine.

I spent some time assessing today. A fair bit of maintenance and upgrades would be needed to get this addon caught up.

Given that ember-data is changing significantly in the 4.x series I'm not personally willing to invest the effort to complete this.

The project which depends on this addon (in my case) only uses the FastbootAdapter mixin, so I will port that code manually as it's fairly small.

Also, this feature is specific to Ember 4.x?

This feature (implicitly injecting a service into all objects) is a no-op with a console warning in Ember 4 which has moved towards more explicitness in general.

I've seen maintainers treat this as a breaking API change as for consumers < 4.x it will remove automatic injections.

@ryanto
Copy link
Member

ryanto commented Dec 19, 2022

Ok cool, thanks for the info. If you're only using the fastboot adapter then ya I think it makes to do that port on your own.

@st-h
Copy link
Contributor

st-h commented Jul 3, 2023

@ryanto would you please reconsider merging this? 0.18.2-ember4.0 does not work with ember 5.0. I just tried to import this PR "ember-data-storefront": "git+https://github.com/gilest/ember-data-storefront.git#chore/remove-implicit-injection", and it looks like it works fine with ember 5 - I am mostly just using the FastbootAdapter and loadableModel however.

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

Successfully merging this pull request may close these issues.

Remove implicit injection
3 participants