Injection Hook Normalization

[pzuraq]

Rendered

[dfreeman]

I'm sure a lot of thought has gone into the form factor here, but I'm going to offer my less-deeply-considered 2¢ anyway 🙂

My gut reaction to this proposal is that normalizing injection across the board is excellent for API consistency, but I have a fairly strong preference for something akin to @start and @teardown over didCreate and willDestroy.

One of the things I really like about the Glimmer component API is that it relieves a lot of the cognitive burden that the classic API placed on developers in the form of special method names they needed to be aware of. Moving those things to be explicit opt-in annotations in the template eliminates a lot of implicit "magic" and makes it much easier for someone new to Ember to understand why a particular method is being called.

Explicitly annotating methods that should be invoked as part of the container lifecycle feels like the moral container equivalent of the move to render modifiers for component lifecycle events. Putting aside the hypothetical @glimmer and constructor parameter @service decorators (which I agree would be a much bigger shift), it feels like @start and @teardown could fit neatly into the injection system we have today?

[rtablada]

I'm not a fan of two phase setup (constructor then some post-DI function). It feels against the grain of slimming down the API that users should have to learn. I would prefer to keep things to constructor only (whether by decorator, type inference/annotation, or more in the fashion of #451 (updating Ember constructors to accept owner as the first constructor argument).

From a more small details perspective adding didCreate to Glimmer classes could cause confusion since it can easily be mixed up with element lifecycles which are now moved to modifiers.

[btecu]

If DI and properties are not ready until after the constructor, then it would seem that the constructor would be useless in most cases, right?

[pzuraq]

@btecu the constructor would still be implicitly used for setting up class fields, and more generally for setting up the class instance - e.g. shaping, setting initial properties, etc. But yes, in most cases users would likely want to use didCreate.

@rtablada that's definitely a fair point, I agree the confusion would be annoying. I think the long term goal here is that we would get to explicit constructor injections:

class Foo {
  constructor(@service store) {
    //...
  }
}

Which would give users control over the DI in their classes, instead of locking us into an API where we always pass the owner as the first parameter. Unfortunately, constructor decorators and parameter decorators don't exist yet, so this RFC tries to find a middle ground that 1. enables us to do post-creation setup today and 2. does not explicitly require a specific API from the constructor.

I think @dfreeman's point makes some sense in that regard - it's a larger delta, but adding decorators that allow you to specify if a function is a hook at least means users can name the hooks whatever they want:

class Foo {
  @start
  setupWebsocketConnection() {}

  @destroy
  teardownWebsocketConnection() {}
}

Or, alternatively, we could standardize on always passing the owner as the first parameter to constructors, as proposed RFC #451.

[jenweber]

I think the "How we teach this" section would benefit from a quick writeup of what the API docs entry should be, plus showing what an example in the Guides would be updated to look like. It's a little hard to tell currently what the proposed solution is.

[luxzeitlos]

what is .create is manually called but ownerInjection is provided?

[pzuraq]

If someone manually creates a class like this, then it is not being created by the container, so it will not trigger the conatiner lifecycle hooks

[Gaurav0]

I am definitely against breaking glimmer components at this point, even if it is not technically a violation of semver.

[pzuraq]

We ended up updating the Injection Parameter Normalization RFC and FCPing that, which means we will not be breaking GlimmerComponent. This RFC will be updated in the near future to reflect that.

[jordpo]

I really like the flexibility that the decorators afford with native classes and splitting out the setup/ tear down logic related to DI.

[piotrpalek]

I feel like this is trying to fix an issue that Ember created for itself. It's very confusing even for someone who used Ember for about ~3 years.

3 ways to init an object, constructor, init and now Symbols + decorators? Ember already has a big and confusing API imo, and is spreading itself thin with things like forcing itself to support older APIs in a place where we could nicely break up with all of the cruft.

Look at how React introduce hooks. Why not do the same with ES6 classes? Still support the old way of doing things when using extend and create, but if you want the new model, use ES6 classes.
I'm not sure if this is technically feasable, but boy would this let us cleanup.

My main issue with this proposal is probably that it compromises the future API of Ember (by making it overly complicated), just to satisfy what I would call "legacy API".

[raido]

While these new decorators would solve the issue. I think having a mild breaking change here would be easier to accept instead of increasing API surface.

[pzuraq]

@piotrpalek the discussion we had at the face-to-face actually reflected a lot of what you just said. There were a lot of concerns that this was too complicated as a user facing API, and that it would be problematic to teach and convert forward too.

What came out of that discussion is that for Ember base classes and objects, such as Components, Routes, Services, etc. we want to have a simple, uniform API for component lifecycle. We landed on:

• constructor will be passed the owner like in Glimmer components today, so that the constructor is the one true way to setup an Ember class
• willDestroy will be the method for tearing down the class

You can follow that in #451, which has been FCP'd.

However, building our DI system around this contract is not super flexible, should it need to change in the future. It also means we can't easily inject any other code from the JS ecosystem. For instance, say you wanted to register a Redux store - you would have to create an Ember class that wrapped the store, which is probably less than ideal (more code + maintenance work whenever APIs need to be updated).

This RFC will be updated soon, but the idea now is that these decorations will be low-level APIs that we use to setup Ember's base classes, but can also be used by advanced users to setup their own classes that are registered on the container.

[piotrpalek]

@pzuraq This design speaks to me :) I don't understand the drawbacks entirely. When you say "create Ember class" you mean one based on EmberObject right? So when I want to register the Redux store, couldn't I register it in an initializer, and the explicitly inject it via a decorator?

[pzuraq]

The contract in #451 is actually a bit more general than that. It means that for all classes that we provide to users to extend, this is the API that the class will have. Glimmer components, for instance, don't extend from EmberObject, but they do have the same API.

As for the Redux store, the issue at the moment is that you would need to extend the store and add a static create method, since the container currently always expects that method to exist. This system is a bit inflexible as is, and makes something that should be pretty straightforward a bit more difficult is all. There's also discussion of creating a factory manager, which would handle the instantiation of factories/classes in general in the container, and could overlap with this RFC quite a bit. I think we'll probably let this RFC bake for a longer period while we nail down the specifics, and moving forward with #451 allows us to do that 😄

[piotrpalek]

Thanks! I think I get it now. I'll wait for the updated RFC with more questions :)

[rwjblue]

We discussed this in todays Ember framework meeting, and are 👍 to move this into final comment period.

DANGIT! Sorry y'all, totally messed that up, I crossed my wires...