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

Add inject service RFC #752

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Jun 10, 2021

@kpfefferle
Copy link

kpfefferle commented Jun 10, 2021

This may seem like a good idea in isolation, but it breaks down a bit in the greater context. A couple of immediate issues that I can see with directly renaming inject to service:

  1. The default export from the @ember/service module is typically named Service (as it's the base class to extend when creating a service). If you're injecting a service into a service, you'll end up with a import statement that looks something like this that's likely to cause confusion:

    import Service, { service } from '@ember/service';
    
  2. The name of an export like inject also needs to make sense internally within the @ember/service module. This is the primary reason why inject is the most logical name for both Service and Controller inject functions - because that naming describes exactly what they do.

I have had some apps I've worked on where both inject functions are imported into the same file (though IMO Controller injections should generally be avoided). In these cases, I rename them to injectController and injectService instead of controller and service. If we really want to push to rename the exported functions so that we can avoid renaming them on import (which has not really been a pain point for me personally), then maybe injectController and injectService would be better naming choices as they'd satisfy both of the above concerns?

@NullVoxPopuli
Copy link
Contributor

that looks something like this that's likely to cause confusion:

I don't think this'll cause confusion

I'm in favor of this RFC -- a very much don't like renaming the import as a default for convention's sake. It'd be fantastic to just import service

@NullVoxPopuli
Copy link
Contributor

though, it might make more sense to have

import { service } from '@ember/application';

@pzuraq
Copy link
Contributor

pzuraq commented Jun 10, 2021

Personally, I think that decorators should describe what they provide, not how they provide it. @inject is a verb, it's an action that we take at a specific point in time. In declarative programming, we want to avoid describing values imperatively. @service describes what the value is, and how it should be thought of when used.

I also don't think the overlap between injecting controllers and services should matter much. Controllers will eventually be deprecated, along with controller injections, so we should be focusing on the future of dependency injection without them, which will likely consist of only services.

@chriskrycho
Copy link
Contributor

@kpfefferle a couple notes in response here:

  1. While it's true that you could have confusion from import Service, { service } from '@ember/service';, I suspect the vast majority of injections aren't in other services, but in components etc. And of course you can still rename if it's helpful in that case!

  2. There's zero reason—literally none!—that the name used internally and the exported name need to be the same. To the contrary! A module can and should use whatever name makes the most sense internally, and then export for its public API the name that makes sense for consumers. JS supports this as a language level feature!

    export { inject as service };

@mydea
Copy link
Contributor Author

mydea commented Jun 10, 2021

I personally have always been using

import Service, { service } from '@ember/service';

when I needed another service inside of a service and never found it confusing.

I also never inject controllers, I just notice it every time I type @inject in my editor to get auto-completion and always get two different imports with the same name proposed, having to look closely at the import path in order to pick the correct one. (only to then go in and manually alias the import)

@chancancode
Copy link
Member

This was discussed at today's framework team meeting and we think this is ready for Final Comment Period!

@mixonic
Copy link
Member

mixonic commented Jul 14, 2021

I've pushed a notable update here. It:

  • Makes the 5.0 deprecation timeline for inject explicit.
  • Adds prose about how addons need to handle this adjustment.

@rwjblue rwjblue merged commit eb25ca8 into emberjs:master Oct 8, 2021
@rwjblue
Copy link
Member

rwjblue commented Oct 8, 2021

Thank you!!!

@buschtoens
Copy link
Contributor

Thanks for working on this RFC! I'm happy to see it merged.

With regards to addon interoperability with older Ember versions, I was wondering whether we could provide a polyfill instead of requiring addon authors to spell out:

import * as ES from '@ember/service';
const service = ES.service ?? ES.inject;

In old globals-resolver-based apps we could probably leverage babel-plugin-ember-modules-api-polyfill. In apps using the modules API and / or embroider, we probably need some other mechanisms, e.g. macros or import maps.

@mehulkar
Copy link
Contributor

I see a bunch of comments in here about deprecating controller injections and just wanted to share #574 again. Feel free to resurrect.

Windvis added a commit to redpencilio/ember-inject-service-import-polyfill that referenced this pull request Jan 9, 2022
This adds a Babel plugin which renames `service` imports to `inject` if the current Ember version doesn't support this import yet.

This is a polyfill for RFC 752. More information can be found in the RFC PR: emberjs/rfcs#752
Windvis added a commit to redpencilio/ember-inject-service-import-polyfill that referenced this pull request Jan 9, 2022
This adds a Babel plugin which renames `service` imports to `inject` if the current Ember version doesn't support this import yet.

This is a polyfill for RFC 752. More information can be found in the RFC PR: emberjs/rfcs#752
Windvis added a commit to redpencilio/ember-inject-service-import-polyfill that referenced this pull request Jan 31, 2022
This adds a Babel plugin which renames `service` imports to `inject` if the current Ember version doesn't support this import yet.

This is a polyfill for RFC 752. More information can be found in the RFC PR: emberjs/rfcs#752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants