Helper Managers

[pzuraq]

Rendered

[chriskrycho]

This is good, but I think probably somewhat incomplete. These should certainly be covered in API docs, but API docs are insufficient for actually building the appropriate mental model for consumers. (Both the CLI and Data docs here are prime examples of this problem historically for Ember! Even when they had full coverage of the API surface, it has been difficult to learn or master.)

Effective documentation includes not only reference materials but also tutorials (introducing the ideas in the first place), how-to-guides (like “cookbooks”—solving specific problems), and explanations. This RFC itself would suffice for the explanation, and API docs will handle reference materials, but I think we should also identify what appropriate kinds of tutorial and how-to material are appropriate here—presumably in an Advanced Guides section for addon authors specifically.

[pzuraq]

The issue here is this is not even a "standard" advanced Ember topic. I would say the current advanced section (In-Depth Topics) is currently stuff that most intermediate and senior Ember devs will eventually need to know or learn. Ideally, most Ember apps will never include a custom manager directly, and most senior Ember devs will not have to write one.

I think a guide would be useful, and I'd be happy to write one for managers in general, but I'm not sure where that would live. Maybe "Advanced Topics" would be good to add to the guides, we've shied away from the term because we didn't want to discourage beginners who are curious (thus the name "in-depth") but this would be a time when that would make sense.

[locks]

Just wanted to pitch in that this is something the learning team has considered previously and recently discussed at one of the meeting. If there's interest enough interest to kick-off an RFC…

[chriskrycho]

I’d be happy to co-author, @locks! I actually mentioned as much to @pzuraq in a DM very recently!

[rwjblue]

The prose does not define if this hook is auto-tracked. I personally think it should be (but I vaguely recall that we may disagree here?), but regardless of which way we go the design should explicitly call out if it is or is not auto-tracked.

[pzuraq]

The issue with autotracking the creation hook is that it would break precedent, and we don't really have a clear meaning for it. We could call getValue or runEffect the next time that dirties, but it is already effectively "dirty" because we will call those hooks anyways, and we don't want to teardown and recreate the helper because then we're taking control away from the user (they can no longer control their own tracking semantics).

Instead, users can use the cache API to wrap any code that they want to autotracking in this hook, and they can do anything they want with that information. They could return a cached value in the first getValue, they could destroy and recreate the instance of the helper, etc.

Really, createHelper just means "you exist in the template now", IMO. We shouldn't define any semantics for it beyond that.

[rwjblue]

FWIW, I wasn't trying to convince you to change it (though I think we should 😉), but the fact that it is missing from being explicitly stated is my fundamental problem here.

Not saying that it is or is not expected to be auto-tracked will cause people to file bug reports when they expect one version or the other and are surprised. In an "auto tracking world", when things are intentionally not auto-tracked it should be clearly stated.

[pzuraq]

I added a section noting that the createHelper function is not tracked, and strategies for how users can track instance creation if they want to. We can keep discussing whether or not the hook should be tracked, I think the biggest question there is if it is, what would it do when it invalidates?

[rwjblue]

I think the biggest question there is if it is, what would it do when it invalidates?

Ya, this is exactly the question.

@krisselden - I think you had thoughts here, what do you think?

[rwjblue]

per helper factory

I don't think the term "helper factory" is actually defined here (this is the only reference in the whole document). Would helper definition work here?

[pzuraq]

hmm, I think it was supposed to be "one helper manager per helper manager factory". The semantics are that we weakmap the function that produces the manager to the instance of the manager, so you could use the same factory function with many different definitions, and they would all use the same instance. That's the thing we need to capture here.

[rwjblue]

Ya, my concern wasn't about that coupling, but it was with the terminology used to talk about which thing the factory function is associated with. There is no conceptual thing introduced in this RFC as a "helper factory". There is a HelperDefinition (which is what this argument is actually called throughout the rest of this document), but no "helper factory".

Also, conceptually calling this a "helper factory" is misleading. There is no reason for us to infer that this value is a "factory" at all (factory usually inferring that you can do new Foo() or Foo.create() in Ember parlance). For example, it is perfectly valid for this value to be {}.

[pzuraq]

right, but again, I'm not talking about the definition, I'm talking about the function that creates the helper manager itself. That is a different thing entirely, which we need a new term for, it is not the HelperDefinition

[rwjblue]

Hmm, why would we do a different thing here than for component managers / modifier managers? We do not cache the factory instance on the manager factory function, we call the manager factory function for each component base class we find.

See this demo repo if you'd like to fact check me 😜:

https://github.com/rwjblue/manager-invocation-demo

[pzuraq]

Ah, this is my mistake then. This is how managers work in Glimmer.js currently, I must've forgotten where we put that optimization. It looks like it first came from ember-functional-modifiers, which makes sense because that addon needed to assign the same modifier manager to many functions and we wanted to avoid creating an instance per function: https://github.com/ember-modifier/ember-modifier/blob/master/addon/-private/functional/modifier.js

Personally I think this optimization is common enough that we should consider making it the default, but we can make it something users have to implement if they need it. I'm not sure if it would be a breaking change to update the existing managers - in theory, they shouldn't depend on instance state per-definition (they may depend on state overall, or per owner), that would somewhat defeat the purpose.

[rwjblue]

Personally I think this optimization is common enough that we should consider making it the default

Ya, I think that might be fine, but adding caching for something that is super light weight (new Something() where that constructor does basically nothing) is actually going to make this slower a lot of times.

Even if we do this, the prose here is wrong. It is not one per manager factory function, it is one per manager factory function per owner. I think that is conceptually fine though, just needs tweaking in the prose to be clearer I guess.

[rwjblue]

If we do that, we should also consider doing the same for the other manager types. I'd hate to introduce another small difference while our overall goal is to reduce the differences. I think we can just do the implementation change for the others though, I don't particularly think it is a breaking change or anything.

[rwjblue]

Can you add some prose to clarify what the behavior is in SSR / FastBoot? As you know we do not run modifiers in SSR, and in general SSR does not have the ability to run update opcodes. I could easily imagine that we say runEffect should not run in SSR (essentially saying that you shouldn't have external side effects), but we should be clear and we should consider it carefully.

[rwjblue]

I think we probably should say they don't run in SSR for now. I can absolutely imagine wanting them eventually in SSR, but I think the safe default to start with (especially while introducing this new concept) is to disallow it. In the future, we can add a capability to explicitly declare that you do or do not want to run in SSR which means this isn't really blocking future functionality.

Primary issues addressed!

[rwjblue]

We discussed this in todays framework meeting, and are in favor of moving forward here!

Look out! It's final comment period time!! 💬 🎭

[rwjblue]

ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG ZOMG

LETS DO IT!!!!!