-
-
Notifications
You must be signed in to change notification settings - Fork 70
Add deprecation guide for Ember.Evented and @ember/object/events #1404
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ember-deprecations ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
this.#emitter.emit('loggedOut', oldUser); | ||
} | ||
|
||
// Public subscription methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, maybe
on(event, callbark) {
return this.#emitter.on(event, callbark);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but what you have is easier to be type-safe
Co-authored-by: NullVoxPopuli <[email protected]>
|
||
Please note: The methods from `Evented` (`on`, `one`, `off`, `trigger`, `has`) were also available on `Ember.Component`, `Ember.Route`, and `Ember.Router`. While usage on these objects is deprecated, the methods will continue to be supported and not deprecated on the `RouterService`, since key parts of its functionality are difficult to reproduce without them. | ||
|
||
### Replacing `Evented` with `emittery` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, for any application at scale this is going to be a pretty huge change. Switching from synchronous events to asynchronous events is almost certain to not just be a drop-in replacement like this, and this deprecation guide doesn't mention that at all.
It looks like this deprecation guide is simultaneously doing two things:
- Describing the deprecation of
Evented
, and providing a possible path away from it - Recommending that users make the architectural change of switching from synchronous event emitting to asynchronous event emitting.
I think these really need to be called out separately. Users should be aware that if they just follow this deprecation guide mechanically they'll be changing timing characteristics of their application in ways that can (and in many cases almost certainly will) break things. So I think we need to be more explicit that we're both recommending a new library to use and the change from synchronous to asynchronous events, and ideally offer up a recommendation of a synchronous eventing library for users that aren't ready to make that change but still need to deal with this deprecation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this callout. I would have thought existing events are async via the runloop. However, if we're sure this isn't the case, then we can definitely call it out in this guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observers are async by default, @ember/object/events
and Evented
are synchronous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the guides to make this clear.
Is there a deprecation plan for But deprecating This guide seems to be addressing cases where |
@bendemboski I've updated the RFC to match this deprecation guide. |
RFC: emberjs/rfcs#1111