RFC: public types for Owner, Transition, RouteInfo

[chriskrycho]

From the summary:

Introduce public import locations for type-only imports which have previously had no imports, and fully specify their public APIs for end users:

• Owner, with TypeOptions and FactoryManager
• Transition
• RouteInfo and RouteInfoWithAttributes

rendered RFC

[MrChocolatine]

Awesome, thanks @chriskrycho.

[wagenet]

Owner is the optional first argument for EmberObject as well.

[rwjblue]

It is not clear to me that we want resolveRegistration at all, I'd expect that given that it predates factoryFor and that (AFAICT) you can do everything you want with .factoryFor that it's just a better replacement...

[chriskrycho]

Excellent; will swap those!

[rwjblue]

FWIW, I actually don't think this is quite correct. The "Factory manager" is the thing that is responsible for actually making .create() function, but I don't think we want the mental model of folks to be that "owner.factoryFor returns a factory manager".

A few issues with the term "factory manager" in a public API sense:

1. The "manager" thing is this:

https://github.com/emberjs/ember.js/blob/55ffe6326f11efcaeb278cdf7e0f86543daa9f04/packages/%40ember/-internals/container/lib/container.ts#L462

But really, you can only actually use .create/.class (like you mentioned here), which is importantly quite different from a "manager" (conceptually).

https://github.com/emberjs/ember.js/blob/55ffe6326f11efcaeb278cdf7e0f86543daa9f04/packages/%40ember/-internals/container/lib/container.ts#L243-L248

2. Our lexicon already includes a thing called "managers" (think component managers, modifier managers, helper managers). Those things augment object's to control creation, provide hooks and capabilities, etc. All of those "managers" are structured very very different that the thing you are referring to here.

---

All that said, I think we just need a different name than FactoryManager. Probably "bad", but also not terrible FactoryForResult or Factory (considering that "factoryFor" should conceptually return a "factory").

I don't think the name itself is a massive ergonomics issue, since I doubt most folks are going to actually write this name themselves (since they will interact with it as a result of calling .factoryFor). We just need to make it not collide / confuse other concepts.

[chriskrycho]

I'm entirely fine with going back to Factory here, FWIW; we should make a point to update the docs for factoryFor accordingly! Will push an update to that effect.

[chriskrycho]

Updated to use Factory in c71d685! And called out the corresponding update to the docs.

[chriskrycho]

@rwjblue this is almost as bad as Engine and EngineInstance and their Application friends: it turns out that we have FactoryManager extensively throughout, and that there are two types in play:

• Factory, which is what register takes and also what lookup (may) return (though in the latter case, not in a way we can currently provide useful types for 😭—it just has to be unknown)
• FactoryManager, which is what factoryFor returns (exceedingly confusingly in my opinion!)

Why in the world factoryFor returns the thing which manages factories for things is likely a 😭 moment in the depths of time, but that's what it actually is. I'm open to names for the latter, though FactoryForResult is 😬 to me.

[chriskrycho]

I'm actually going to call it FactoryFor! It is indeed a type which relates two other types, so that name makes reasonably good sense here.

[chriskrycho]

…okay, having read the implementation and the docs repeatedly, I now: have literally no confidence that my idea of how these types are related or how they are supposed to be used is correct. Factory and FactoryManager have very similar APIs, but FactoryManager#class is a Factory whereas Factory#class is a "class" which can either be any old object (?) or specifically a FactoryClass, which is just any object with positionalParams of a certain type?

Writing this down for now and will come back to trying to actually get something reasonable after discussing with folks who know this more (@rwjblue, @wagenet, etc.) tomorrow. 😂

[rwjblue]

@chriskrycho - How can we ensure that folks don't try to leverage these new (type only) imports in a runtime capacity? I don't see that being addressed...

[chriskrycho]

@rwjblue it's called out as part of the definition as well as in the How We Teach This section for Transition and RouteInfo; see here. We will also want to double down on that idea as part of the blog post called for in that section, since it's a new concept to not only the Ember community but the JS community at large.

[chriskrycho]

I have just pushed, in 5b9bc9c, a proposed name for the thing currently named FactoryManager in Ember's internal types: FactoryFor. It now at least has a well-defined type… and we can bike-shed the name, because I hate that name!

[chriskrycho]

At the Framework team meeting on Friday, we discussed this a bit and had two takeaways:

1. The tentative conclusion around FactoryManager (FactoryFor) was to keep it named FactoryManager with a plan to add the capabilities hooks etc. to normalize it into being a normal *Manager the way the rest of Ember’s/Glimmer’s managers are.

2. Owner should be a public type, but not a user-constructible type: because that requires end users to implement the whole API surface, including providing a FactoryManager—which does not seem reasonable, at least yet. (Also, per the SemVer for types spec, note that adding a non-optional field to a user-constructible interface is a breaking change!)

   Existing usage of the Owner interface this way (e.g. setting custom owners for tests) mostly falls under the "intimate API" rules, and will likely be deprecated after a future introduction of createOwner() hook so that that there is a public API way to get the required type.

There are no concerns about the other API types introduced here. I will update the RFC and expect to bring it back to put it into FCP on Friday!

[chriskrycho]

We talked about this at today's Framework meeting and agreed that it's ready for FCP, and the Typed Ember team agrees, so this RFC is now in FCP!

[chriskrycho]

We talked about this at today's Framework team and are merging it. Types will be updated on DefinitelyTyped ASAP!