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

Injection Parameter Normalization #451

Merged
merged 8 commits into from
Apr 12, 2019

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 20, 2019

@rwjblue rwjblue self-assigned this Feb 20, 2019
@rwjblue rwjblue added the T-framework RFCs that impact the ember.js library label Feb 20, 2019
@Gaurav0
Copy link
Contributor

Gaurav0 commented Feb 21, 2019

Oh jeez. The Ember 3.6 update was already painful for me, and we weren't using as many native classes as we are now.

Also, I actually think this makes things more complicated than they are now. Right now, if it extends Ember.Object, use init, otherwise use constructor. I think this RFC makes it more complex deciding which to use.

I know this is supposed to make things easier to teach for new users, but I don't think it will, Most new users will still have to deal with classic components and utility classes for some time because they maintain code at companies or they use Ember addons.

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 21, 2019

Right, that’s why we’re planning on having a strong lint workflow that helps users know which one they should use for every class until it has been upgraded.

The workflow is becoming more and more necessary as we go along. For instance, when a user jumps into a component file, are they looking at an classic component or glimmer component? You could look at the import path, but what if the import is for a base class that you’re extending? It’s a lot of work either way, and will be frustrating unless we can catch it early with a linter.

FWIW, the changes proposed are also fully backwards compatible, unlike the changes in 3.6. I am very sorry about those, and about the extra churn 😔 but I do believe this move sets us up for success. If you want, you can keep using the init hook until all other classes have been converted from EmberObject, and then universally switch to constructor.

@tomdale
Copy link
Member

tomdale commented Mar 8, 2019

We discussed this at the core team meeting today and, given that the conversation has stabilized, we agreed to move this RFC to Final Comment Period.

@krisselden
Copy link

krisselden commented Mar 8, 2019

I believe if we made the container system have decorators, same with component manager, we could move off of the base class with minimal effort in the container.

// DI container
const Ctor = lookup(desc);
const obj = Reflect.construct(Ctor);
setOwner(obj, owner);
placeInContainer(obj, desc);
Object.assign(obj, propInjections);
invokeStartIfNeeded(obj);
return obj;

No base class

import { start, teardown } from "@ember/container";
import { tracked } from "@glimmer/tracked"
import { glimmer } from "@glimmer/component"

export default class ClockService {
  time = Date.now();
  #interval = undefined;

  @start
  start() {
    #interval = setInterval(() => this.time = Date.now(), 1000)
  }

  @tracked time;

  @teardown
  destroy() { 
    clearInterval(#interval);
  }
}

export default @glimmer class MyComponent {
  @service clock;

  get time() {
    return this.clock.time;
  }
}

@rtablada
Copy link
Contributor

@krisselden I like the idea of future lifecycles being decorated instead of inherit to the base class (and possibly remove the need for an Ember base class in many cases)

@rwjblue
Copy link
Member

rwjblue commented Mar 13, 2019

@krisselden - I can't tell if that is a counter proposal, or supporting this RFC by saying we can move away from this in the future by way of decorators. Can you clarify?

@krisselden
Copy link

@rwjblue I disagree with the idea that having a new Service base class that receives and just does setOwner(this, owner) is the future to migrate people to. So, yes, I'm objecting.

@rwjblue
Copy link
Member

rwjblue commented Mar 14, 2019

@krisselden - Gotcha, thank you for clarifying!

@pzuraq pzuraq changed the title Classic Class Owner Tunnel Injection Parameter Normalization Mar 15, 2019
@pzuraq
Copy link
Contributor Author

pzuraq commented Mar 15, 2019

I've opened up #467 as a potential alternative to this RFC, following discussions with @krisselden about his concerns. I believe either of these RFCs is better than the status quo, but I would probably agree with Kris that normalizing injection hooks is the better path for the future, even if it means changing the Glimmer component API.

@chancancode
Copy link
Member

Based on the objection raised by @krisselden, I'm taking this RFC out of FCP pending further discussions.

@tomdale
Copy link
Member

tomdale commented Mar 29, 2019

We have updated the RFC to incorporate the most recents concerns and agreed to move this RFC back to Final Comment Period.

@pzuraq pzuraq mentioned this pull request Mar 31, 2019
@chriskrycho
Copy link
Contributor

@tomdale or @krisselden can one of you (or someone else!) summarize the team's current take on this design in response to Kris' comments above? It's sort of possible to infer based on the latest diff, but it'd be really helpful to have a little more explicit clarification. 😄

@chriskrycho
Copy link
Contributor

Also, @pzuraq, can you please update the Rendered link at the top to point to the branch instead of the commit? As it is, it's pointing to the version from before the most recent update!

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 2, 2019

@chriskrycho I think I can summarize actually.

@krisselden was primarily concerned that we were encoding the contract for the constructor for all classes into the container. Based on his experience and knowledge of other DI systems, and their battle-tested patterns, he thought that really enforcing any kind of contract for all classes would be a bad idea. Most mature DI systems rely on some form annotation, decoration, or abstraction in general to separate the creation of classes and how they receive injections. This allows them to be very flexible, and is the basis for the Injection Hook Normalization RFC #467.

In the discussion, we that a more general system like the one proposed in #467 would be ideal for the DI container itself, but that doesn't mean that your average Ember user has to use decorators/hooks/etc to define these things in classes that we control. Framework classes, like Service, Controller, and Route, could be defined using the annotation to setup conventional hooks and behaviors:

@injectOwner
export default class Service {
  constructor(owner) {
    setOwner(this, owner);
  }
}

This gives us the best of both worlds. Average users who are extending standard framework objects don't need to worry about the details - they setup their class in the constructor hook, and tear it down in the willDestroy hook. Experienced users who want to register something new or unusual in the DI system can use the lower level annotations (or factory manager, or what have you) to get exactly the behavior they want.

This also gives us more time to bake the lower level system. It makes it an implementation detail for the user facing API, which is really the higher-order bit at the moment, and what this RFC formalizes.

text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
text/0000-injection-parameter-normalization.md Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the classic-class-owner-tunnel branch from c6b0027 to d2d078a Compare April 6, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants