Enable global event dispatcher listeners to be lazily created.

[simonihmig]

Fixes #15140, supersedes #17911, makes emberjs/rfcs#675 unnecessary (see #15140 (comment))

This is based on the previous work by @rwjblue in #17911. Lazily copying his description here:

Prior to this change, all events that would ever possibly be used would be eagerly setup by the event dispatcher's setup method. Unfortunately, this has (at least) two major downsides:

• As of Chrome 51, adding listeners for touch events (touchstart, touchmove, touchend, etc) without the passive flag issue a performance focused warning. See https://www.chromestatus.com/feature/5745543795965952 for more details.
• A number of the events that we have historically listened for fire massive numbers of events (mouseenter, mousemove, etc) and most applications do not really use these events.

The two primary entry points into using the event dispatcher are the action element modifier and Ember.Component's implementing the "event delegation" methods.

This commit enables the event listeners in the event dispatcher to be lazily setup on demand by updating the action modifier manager and the curly component manager to invoke the event dispatchers setupHandler method lazily when needed.

Updates to his PR:

• rebased to current master
• fixed event handling of built-in components (<LinkTo> and TextSupport based <Input> and <TextArea>, see below for caveats...
• Fix broken use of browser event names vs. (camelCased) Ember event names. Exposing new EventDispatcher#setupHandlerForBrowserEvent and EventDispatcher#setupHandlerForEmberEvent methods for more convenient (internal only!) use
• Fixed EventDispatcher test - see inline comment!
• Added typings for EventDispatcher for props/methods supposed to be used internally, not for private ones in the OOP-sense (everything related to EventDispatcher is still private in a SemVer sense)
• Added support for Evented-based event listeners (this.on('click', ...)), closing the unresolved issue below
• Add test coverage for dispatching all events on Ember components, either through event handler methods or listeners

Unresolved issues

As can be seen in the changes required here for <LinkTo> or <Input>, the new lazy behavior is a breaking change whenever someone used Ember.Component's Evented mixin for adding DOM event listener like this.on('click', this.handleClick), see this example of TextSupport

I can't remember to have seen this documented for attaching browser event listeners, so not sure if this is supposed to be a public API we must not break? If so, we have to find a way to somehow sync the use of this.on() to EventDispatcher#setupHandler.

Therefore marking this PR as WIP as long as this is not sorted out. Otherwise good for review!

[simonihmig]

I was not able to keep this test unchanged, as the reference to the EventDispatcher of the Glimmer Environment (already introduced in the initial PR) would always refer to the first instance, not the second one created here. So the curly component manager would still see the old dispatcher, that has no custom events added.

But I think this affects only our internal testing here, and not imply any publicly visible changes.

[rwjblue]

As can be seen in the changes required here for or , the new lazy behavior is a breaking change whenever someone used Ember.Component's Evented mixin for adding DOM event listener like this.on('click', this.handleClick), see this example of TextSupport

I can't remember to have seen this documented for attaching browser event listeners, so not sure if this is supposed to be a public API we must not break? If so, we have to find a way to somehow sync the use of this.on() to EventDispatcher#setupHandler.

I think something like this would work (for @ember/component):

Component = CoreView.extend(...allTheMixinsInTheWorld, {
  on(event) {
    let eventDispatcher = getOwner(this).lookup<EventDispatcher>('event_dispatcher:main');
    if (event in eventDispatcher.lazyEvents) {
      dispatcher.setupHandlerForBrowserEvent(event);
    }

    return this._super(...arguments);
  }
});

[rwjblue]

I don't think we can use Array.from or includes due to IE11 issues

[simonihmig]

Yeah, already noticed that it breaks the Browserstack CI job. Will fix it later today...

Also .values() btw

[simonihmig]

I think something like this would work (for @ember/component)

@rwjblue Thanks for the feedback. I added something like that, though we have to look for Ember's event names instead of browser event names. That's something which was slightly confusing throughout the whole PR.

So as I was not entirely sure what kind of event name this.on() would expect (it's Ember's version of it), I added some better test coverage for dispatching all events on Ember components, either through event handler methods or listeners.

Also this latest change let me revert the previous changes to LinkComponent, as that does not need to setup event handlers anymore, as the overridden on() would do that now automatically.

IE11 should also be happy now, as CI is green!

[rwjblue]

Awesome, thank you @simonihmig!

[rwjblue]

Looks like we need a rebase. Probably also need to circle around with @pzuraq about how we can reasonably thread the dispatcher through here now that .extra is removed.

[simonihmig]

Looks like we need a rebase.

Done.

Probably also need to circle around with @pzuraq about how we can reasonably thread the dispatcher through here now that .extra is removed.

I guess the reason we have passed the dispatcher through this extra thing was to give the component manager access to the dispatcher, without having access to owner. However as it turned out after again reviewing the code, I already have extended the curly component class to lookup the dispatcher to add lazy event support for this.on().

So in the last commit added after the rebase I basically moved the previous lazy setup logic from the curly component manager's create() to the component class' init(), which seems pretty much equivalent to me, right?

@rwjblue please re-review!

[rwjblue]

In general this seems good (and definitely does what we need).

I'm a tad worried about slowing down all @embed/component instantiation though. Over the course of running the application I think that we would slowly reduce the number of events in lazyEvents but I don't suspect that we will ever get that to be completely empty (since a large number of things just are not likely to be used IMHO).

[simonihmig]

I'm a tad worried about slowing down all @embed/component instantiation though.

Just to be clear: this is just as much (or less) a problem with the current solution (iterating over the lazy events in init()) as it was in the previous (doing the same in component manager's create()), right?

Over the course of running the application I think that we would slowly reduce the number of events in lazyEvents but I don't suspect that we will ever get that to be completely empty (since a large number of things just are not likely to be used IMHO).

Yeah, I understand and share that concern. Not sure how much of a real life problem this would be, but it's reasonable to try to mitigate this problem as much as possible...

What about this idea: can we remember the component classes we already checked (for event methods), e.g. by having a WeakSet<ComponentConstructor> that we use to skip over a component if we already processed its class (constructor) previously?

That brings up another concern we already touched in your previous PR: our assumption that event handler methods may only be defined on the component's prototype.

<MyComponent @keyPress={{this.handleKey}}/>

Albeit a terrible pattern IMHO, I think this should work now with us checking for event methods in init(), where this.keyPress is already set on the component instance, right?

What we cannot support however is the even more weird case when you set an event handler method on the instance after instantiation. Can we safely assume this was never supposed to be supported? Or do we have to regard this as a breaking change, however unlikely such a pattern is?

If so, we could expose the new laziness here in an optional feature maybe? Though that would lead to less users getting the benefits from this (when they forget to opt into or are not aware), and as such would not be that effective in fixing #15140. And I personally would like to not overcomplicate this PR if possible 😆

[rwjblue]

Just to be clear: this is just as much (or less) a problem with the current solution (iterating over the lazy events in init()) as it was in the previous (doing the same in component manager's create()), right?

Yep, the two strategies are going to have similar / same cost I think.

[rwjblue]

What we cannot support however is the even more weird case when you set an event handler method on the instance after instantiation. Can we safely assume this was never supposed to be supported?

No, I don't think we have to support this.

[simonihmig]

What about this idea: can we remember the component classes we already checked (for event methods), e.g. by having a WeakSet that we use to skip over a component if we already processed its class (constructor) previously?

I tried this out, and it seems to work well, though the performance impact is not significant (see below). See the last commit added!

I'm a tad worried about slowing down all @embed/component instantiation though.

I tested this with a simple test app, that

• has a simple component that renders just minimal markup (<li>), and has a dummy click handler (so worst case in terms of lazy events checking, as the lazyEvents map size does not decrease beyond removing click)
• this component is rendered a thousand times in a single batch, then everything is removed again
• this iteration is repeated a thousand times
• which makes the app render the component a million times

The performance results (duration for those 1M rendered components) are as follows, all averaged over three separate runs:

Ember	Duration
master	176.6s
This PR (without last optimization)	177.5s
This PR (with last optimization)	175,8s

So the results are extremely close, and probably all within the margin of error. The last optimization seems to have a slight positive effect, though still not very significant (given that these ~2s are for 1M component instantiations!). So not sure if these are worth the added code complexity?

[simonihmig]

So as I could have anticipated, IE11 does not like my use of WeakSet. I could probably refactor that to a WeakMap instead. Will do that if we decide we want to keep that latest optimization.

[simonihmig]

@rwjblue I updated this PR to the latest changes on master. The recent GlimmerVM update PR caused some conflicts, as you correctly anticipated. I hope this fix is ok, specifically this change here /cc @pzuraq

Also referring to the performance optimizations (see above), I kept my little performance tweak and worked around the limitation of IE11 not supporting weak sets by (mis-)using a weak map. So CI is green now! 😅

Given that there were some changes and rebases after your last review, you will probably have to re-review? But I would certainly appreciate if we could land this asap, not keen for doing another rebase! 😉

[rwjblue]

Thanks for all the hard work here @simonihmig!

[rwjblue]

FWIW, we have a WeakSet polyfill thingy that we use (which lets us treat it like a WeakSet, but on IE11 is implemented as a WeakMap).

import { _WeakSet as WeakSet } from '@glimmer/util';

https://github.com/glimmerjs/glimmer-vm/blob/master/packages/@glimmer/util/lib/weak-set.ts