Implement Default Helper Manager (RFC #756)

[NullVoxPopuli]

Implementation of emberjs/rfcs#756

Todo:

• PR to the RFC to add an example of existing behavior today where consuming a tracked value in the helper body entangles with helper execution

[NullVoxPopuli]

Error: Could not find module @glimmer/interfaces imported from @glimmer/manager

should interfaces be a dep in manager?

it already is. sus

[rwjblue]

Needs a whole slew of tests 😉

[NullVoxPopuli]

Needs a whole slew of tests

I haven't gotten there yet :p still draft 🙃

[NullVoxPopuli]

@rwjblue ready for review / added some tests. Not sure how many test scenarios you want / feel necessary, so if you think of more that are needed, lemme know

[chancancode]

This one probably requires more focused attention, will come back to it next week

[NullVoxPopuli]

sg, rebased

[NullVoxPopuli]

@chancancode should be green - linting seems to have failed for random networking reasons

[chancancode]

I re-read the RFC but I couldn't tell: are we supposed to always invoke the function with an options argument even when syntactically there were no named arguments passed?

i.e. should {{this.myFunc}} be called as this.myFunc.call(undefined) or this.myFunc.call(undefined, {})?

It seems slightly/somewhat problematic either way – but so far I am leaning towards the former. The latter is more consistent I suppose, but if you are deliberately passing a function takes optional positional arguments that are normally defaulted for you (which there are a lot), then always passing an empty object would be problematic.

On the other hand, I am not sure if we have any precedence of distinguishing between having zero named arguments vs did not pass any named arguments. I think syntactically, the distinction does not exist today. But if/when we add splat syntaxes that would make a difference.

We should pick one carefully; we may want to do a quick amendment PR to the RFC to see if anyone else want to chime in.

[chancancode]

I also can't tell from the RFC:

class MyComponent {
  @tracked name = '...';

  greet(message) {
    return `${message}, ${this.name}`; // <- do we call this function again when `this.name` changes?
  }
}

{{this.greet "hello"}}

Personally I think the answer is yes (and there probably isn't other options given how the system works), but 1. we should have test for it; 2. we should update the RFC to state it explicitly

[NullVoxPopuli]

i.e. should {{this.myFunc}} be called as this.myFunc.call(undefined) or this.myFunc.call(undefined, {})?

I think it should be myFunc({}), because at the call site, it's a common pattern (that I've seen anyway (outside of ember)), to do like:

let options = args[args.length - 1];
// or
let [options] = args.reverse();

giving a stable type is good DX, imo, and reduces the amount of surprise when arguments aren't passed.

.call(undefined, {})?

why call? normal invocation preserves the this of the function (if any), and allows people to entangle with the helper function (an existing behavior of today's helper functions).

I also can't tell from the RFC:
the answer is yes (and there probably isn't other options given how the system works),

yes, autotracking is maintained with helpers today.
but

1. we should have test for it;

there already is a test, (Default Helper Manager) plain functions track positional args and (Default Helper Manager) plain functions do not track unused named args

2. we should update the RFC to state it explicitly

Sounds reasonable (esp since our RFCs tend to be the source of truth for docs until an edition lands).
I'll PR that tomorrow (hopefully)

[chancancode]

I think it should be myFunc({})

See above. Not saying it should definitely be myFunc() or that it wasn't without its own problems, but I think myFunc({}) has pretty obvious problems that should require us to rethink this a bit.

Why .call(undefined, {})?

I was just being clear/precise/pedantic, this already is what's happening. When you write {{this.foo}}, we get fn = this[foo] as a value, and by the time you do fn() the this is already lost. To make it a method call you would need @action or this.foo.bind(this), in which case the .call(undefined, ...) does not affect it as the this is already "baked in" to the value.

yes, autotracking is maintained with helpers today

I am specifically asking about when the tracked state does NOT come from an argument (i.e. from this and closure captures). I don't think that was explicit in the RFC nor were there tests for it.

[NullVoxPopuli]

Thanks for the review, @chancancode -- I think I've addressed everything, and added a todo to the PR description for everything that I'm aware of remainaing (just PRing to the RFC an example showing current behavior of auto-tracking local tracked data accessed from within the helpers)

[chancancode]

yes, I know. that's still how it works today, you access this.trackedThing in a helper defined on your component, and it entangles properly.

IMO we should still add tests for this case for the default manager

just PRing to the RFC an example showing current behavior of auto-tracking local tracked data accessed from within the helpers

Sounds good. I think this is good to clarify, but probably secondary to/not as important as clarifying whether there is or is not an automatic {} in the args when no named arguments are passed. I think that is an important design question that was missed in the original RFC, so it's good to make sure we are arriving at the right conclusion. I think we are both leaning no at this point, but we started with opposite intuitions so that's probably good indication that it's not a trivial thing.

If you are able to open the PR, I can bring it up for discussion on Friday to give it some attention, then we can see if we decided to just merge that PR, or tweet that PR for 1-week FCP, etc.

Thanks for working on this!

[chancancode]

IMO we should still add tests for this case for the default manager

I think the easiest way to do that in tests would be something like

// Probably try to make this a slightly more real
class Greeter {
  @tracked name = '...';
}

let greeter = new Greeter();

let helper = () => return `Hello ${greeter.name}`;

// render, assert

greeter.name = '...something else...';

// assert rerendered

Not sure what is available in the test harness here, alternatively you may be able to use a "tracked object" thing, but you can probably find examples in tests covering this behavior in regular helpers

[NullVoxPopuli]

I think the easiest way to do that in tests would be something like

added test: (Default Helper Manager) plain functions entangle with any tracked data

[NullVoxPopuli]

Types are now greatly simplified. Thanks @chancancode, @chriskrycho, and @dfreeman !!!

[sandstrom]

@NullVoxPopuli Awesome! 🎉