Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable global event dispatcher listeners to be lazily created. #17911

Closed

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 12, 2019

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.

TODO:

  • Fix issue with LinkComponent -- it uses evented interface to handle events instead of implementing a method.
  • Performance test against a few "real" apps (ensuring that the extra work being done per-component is not resulting in worse overall initial render performance)
  • Determine if we would like to have some events eagerly setup and others lazy

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.
Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!

@@ -234,12 +242,16 @@ export default EmberObject.extend({
@private
@method setupHandler
@param {Element} rootElement
@param {String} event the browser-originated event to listen to
@param {String} event the name of the event in the browser
@param {String} eventName the name of the method to call on the view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of arguments has changed, and should be reflected in the DocBlock.

@@ -234,12 +242,16 @@ export default EmberObject.extend({
@private
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes this method less "private" I think, at least "private" in the OOP-sense of being able to call it from outside (remains "private" in the semver-sense that it shouldn't be used in user-land). At least when @ember/jquery overrides the EventDispatcher with its jQuery-based implementation when Ember 4.0 will have dropped jQuery support, it has to properly support that method as it will be called from other places. Just to remind us of this... (see emberjs/ember-jquery#31)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, unfortunately @private here is mostly serving as a marker for semver version of privateness. @ember/jquery is "first party" and will be supported regardless...

// non-interactive rendering (e.g. SSR) has no event dispatcher
if (dispatcher === undefined) { return; }

let lazyEvents = dispatcher._lazyEvents;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to be "semi-public", in terms of @ember/jquery, see the other comment...

@simonihmig
Copy link
Contributor

Determine if we would like to have some events eagerly setup and others lazy

There will definitely be events like click that virtually no app ever will not use, but AFAICT the overhead of lazy setup seems to be pretty neglectable in this implementation here, as once the event handler is set up, any new component instantiation will skip it in the setup loop!?

I certainly hope nobody uses any wild pattern like this, setting up the event handler at runtime, which would stop working I guess:

export default Component.extend({
  // ...
  setupListeners() {
    this.mouseEnter = (e) => { this.doSomething() };
  }
});

While I can't remember this to be said explicitly somewhere, but I hope we can safely assume that event handler methods are supposed to live only on the prototype!? Or at least be present after instance creation (like an event handler created as a native class instance field).

@rwjblue
Copy link
Member Author

rwjblue commented Apr 12, 2019

once the event handler is set up, any new component instantiation will skip it in the setup loop!?

Yes, exactly!

I hope we can safely assume that event handler methods are supposed to live only on the prototype!? Or at least be present after instance creation

Yep, that is absolutely an assumption this PR is making.

@simonihmig
Copy link
Contributor

This can be closed, as it is superseded by #19227!

@locks
Copy link
Contributor

locks commented Feb 5, 2022

Closing as per previous comment!

@locks locks closed this Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants