Element Actions for Glimmer

[zackthehuman]

Port existing element actions code into Glimmer.

[chadhietala]

Are there some tests that can now be ran in both Glimmer and HTMLBars?

[zackthehuman]

@chadhietala Currently there aren't any in ember-glimmer (so no INRU tests, etc.). I have some uncommitted work where I started porting over existing tests from ember.js/packages/ember-routing-htmlbars/tests/helpers/element_action_test.js. I can add them to this PR once they're ported.

[krisselden]

Looks good, will merge when we have a test in place

[stefanpenner]

const

[zackthehuman]

I ported most of the existing element action tests. There are a few tests that I stubbed out and marked with @skip because I'm either not sure how they fit into the ember-glimmer test harness or think they should be cut because they test internals. Would like to get feedback on that aspect.

[rwjblue]

I don't think testing of {{action 'foo' on='change'}} is an implementation detail. Changing the event that is listened to is public API that should be tested.

[zackthehuman]

I guess what I'm getting at here is the original test for this intercepts/inspects the slots inside of ActionManager.registeredActions. I think a better way to test this would be to set up an action for several events, test each one, and assert that it fires for the respective event.

[rwjblue]

yep, agreed

[rwjblue]

I left a bunch of comments, but this is looking really good. I a bit confused about the way multiple actions are stored, why do we need multiple attributes on the element?

[zackthehuman]

@rwjblue The reason action data attributes are stored across multiple attributes in Glimmer is because they are assigned at render and we want to avoid reading attributes at render time. In HTMLBars the data attribute is read via getAttribute in order to be reused.

[rwjblue]

As far as I understand it (haven't dug in too much):

• HTMLBars will read the attribute
  • if present it will reuse the same id. This results in one getAttribute for the second action.
  • if not present it will create a new id and setAttribute. This would be one getAttribute and one setAttribute (2 total operations) for first action on an element.
• This PR for Glimmer2 will always setAttribute twice.

In the existing HTMLBars case, we have the potential of using a single DOM operation whereas with this implementation for Glimmer we will always have two DOM operations. In the worst case with HTMLBars today, there are still only 2 operations. The primary difference between the two being that there is a getAttribute then a setAttribute with HTMLBars, and two setAttributes with Glimmer.

Are we saying that two sets is better than a get and a set (and ignoring the somewhat unusual case of multiple action handlers being more optimized)?

[zackthehuman]

@rwjblue My understanding from the reading of this code (

ember.js/packages/ember-routing-htmlbars/lib/keywords/element-action.js

Line 47 in 783a656

let actionId = env.dom.getAttribute(node.element, 'data-ember-action') || uuid();

) is that a getAttribute and a setAttribute will always take place, per action.

The approach I took for the Glimmer implementation is make the operation write-only -- it never performs a getAttribute at render time (or any other time, but the important aspect is that it doesn't take place during first render). The goal here is not necessarily to reduce DOM operations, but to eliminate reading from the DOM. @krisselden may have more input on this.

[rwjblue]

@rwjblue My understanding from the reading of this code (

ember.js/packages/ember-routing-htmlbars/lib/keywords/element-action.js

Line 47 in 783a656

let actionId = env.dom.getAttribute(node.element, 'data-ember-action') || uuid();

) is that a getAttribute and a setAttribute will always take place, per action.

You are correct, thank you for clarifying that!

The goal here is not necessarily to reduce DOM operations, but to eliminate reading from the DOM.

Right, this is the heart of my question. If the operations are the same, why jump through extra hoops just to avoid reading from the DOM. I kinda assume that there is something fundamental that I am missing...

[zackthehuman]

@rwjblue I believe I have addressed all of your test feedback. Take another look when you have a chance. As respects your getAttribute question -- it's something I was asked to avoid doing at render time. You may have to get more details from @wycats about why this is desirable.

[rwjblue]

Yep, no problem. My comments RE avoiding the getAttribute first were most curiousity.

I'll try to do another round of review tonight or tomorrow morning....

[homu]

☔ The latest upstream changes (presumably dde51f6) made this pull request unmergeable. Please resolve the merge conflicts.

[krisselden]

@zackthehuman can you rebase? also the just use master's package.json, when you rebase master is 'ours'

[krisselden]

@rwjblue did you have any outstanding concerns or can I merge this?