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

Deprecate Events #528

Closed
wants to merge 9 commits into from
Closed

Conversation

ggayowsky
Copy link

@ggayowsky ggayowsky commented Aug 15, 2019

@CvX
Copy link
Contributor

CvX commented Aug 15, 2019

Great RFC! 🙂

Could you update the "Rendered" link? (it points to master instead of deprecate-evented-mixin)

@pzuraq
Copy link
Contributor

pzuraq commented Aug 15, 2019

I'm very much in favor of this RFC 😄 I actually think it could go further. I'm planning on submitting quite a few deprecation RFCs post-Octane, and two that I have planned are:

  1. Mixins, in general
  2. Event listeners (from @ember/object/events), in general

This would be a good first step toward either of these, but it may also be worth expanding the scope here since all of these features are going to eventually be deprecated anyways. If you're interested, we can dive in and discuss what that would look like!

@ggayowsky
Copy link
Author

I am amenable to expanding the scope of the PR and would enjoy discussing further in discord.

I agree with deprecating Mixins entirely. As far and I can tell, the Evented Mixin is the only Mixin that is publicly exposed through the ember packages. I figured that deprecating only Evented is simple first step, that would help reduce churn (compared to deprecating all Mixins) since the Evented Mixin seems to have a very straightforward transition path for deprecation (with potential for a codemod).

If the plan is to deprecate the @ember/object/events package (and not expose as an add-on), then I would want to update this RFC's transition path to include suggestions that will remove dependency on said package.

@runspired
Copy link
Contributor

I'm also in favor of this, and since EmberData has already shipped the deprecation of our own usage of the Evented mixin this shouldn't be terribly difficult: #329

@ggayowsky
Copy link
Author

ggayowsky commented Aug 22, 2019

After discussion with @pzuraq in the discord, this RFC will have its scope expanded to consider deprecation all eventing from ember (Evented, @ember/object/evented and @ember/object/evented and potentially more)

@gossi
Copy link

gossi commented Aug 22, 2019

I want to throw in an alternative to this is to provide a decorator that takes over the job of the mixin. Similar to what orbitjs does: https://github.com/orbitjs/orbit/blob/master/packages/%40orbit/core/src/evented.ts

That can keep the API consistent only the extends EmberObject.extend(Evented) needs to change to a decorator here.

And I'm summoning @dgeb into this.

@pzuraq
Copy link
Contributor

pzuraq commented Aug 22, 2019

@gossi: @ggayowsky and I discussed this possibility in Discord, and it definitely is something that could be done. However, eventing is just not something that is core to Ember anymore. The framework itself does not use eventing internally anymore, and it really makes sense to remove it so we aren't maintaining an orthogonal feature in the same codebase.

We could create our own @evented decorator and maintain it as an official Ember library, but there are already several (non-decorator) eventing libraries that are well maintained on NPM:

Since this is functionality that isn't used very often, I think the best path forward is to point users who need it to these libraries instead so we lessen the maintenance burden on our ecosystem.

@ggayowsky ggayowsky changed the title Deprecate Evented mixin Deprecate Events Aug 23, 2019
@ggayowsky ggayowsky changed the title Deprecate Events WIP: Deprecate Events Aug 23, 2019
@ggayowsky ggayowsky changed the title WIP: Deprecate Events Deprecate Events Aug 23, 2019
@mehulkar
Copy link
Contributor

Should this RFC include the work required to move Ember itself off this mixin? E.g. the Router uses mixes in Evented, as I'm sure do other things.

@locks
Copy link
Contributor

locks commented Aug 24, 2019

@mehulkar that's what the RFC and the thread is about, deprecating the mixin and all the events.

@lukemelia
Copy link
Member

As a reasonably heavy user of the Evented mixin, I’m not opposed to this change but would suggest that we should let the absence of an Ember implementation of eventing stop us from providing a codemod that updates code to use one of the listed libraries. At best, it substantially eases migration risk and at worst it provides an example for developers who want to migrate to a different lib.

@st-h
Copy link

st-h commented Aug 26, 2019

I personally think that removing stuff that no longer is used internally from ember core is a reasonable decision. However, I also think that just removing features will have at least implications on developer experience, as long as we do not provide a clear way forward.

I have started to make use of the Evented Mixin after I noticed that popular addons like ember-simple-auth were using it and found it quite handy. Despite this feature not being promoted heavily on the emberjs website, it seems like this is a feature that is not just very rarely used.

Especially for addons a situation where there is no "official" replacement will likely have implications, as this will encourage addon developers to make use of an arbitrary 3rd party dependency or write their own code. This in terms will bring up a situation for all app devs where they could easily end up with multiple js dependencies that just do the same thing which will only unnecessarily bloat their apps.

I am especially concerned that ember will no longer provide any means to do things like for instance: Make use of a service to wrap a third party event (very simple example the online event). And then allow other parts of the app to subscribe to that event, so they could display the correct state. (yes, we could make every component directly implement the native event, however there are pretty good reasons to not do so)

I think we should continue to provide an official add-on which brings back removed features (as this usually has been the case in the past) to mitigate all those issues, if we want to remove events from ember core.


## Alternatives

* Do not deprecate Ember Events
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Do not deprecate Ember Events

text/0000-deprecate-evented-mixin.md Outdated Show resolved Hide resolved
text/0000-deprecate-evented-mixin.md Outdated Show resolved Hide resolved
text/0000-deprecate-evented-mixin.md Outdated Show resolved Hide resolved
@pzuraq
Copy link
Contributor

pzuraq commented Aug 26, 2019

@st-h you make some great points! First off to clarify, I want to see if I fully understand the concerns:

  1. Eventing is a useful pattern, and it may be used pretty frequently in JS/Ember apps.
  2. Because it is used frequently, it's likely that there will be many addons that require it. Without a unified eventing solution, they will each make there own choice of eventing library, and this would lead to bloat.

I don't think a core issue here is that there isn't a vanilla JS library that provides a good (if not direct) replacement library - i.e. Ember could choose to recommend an existing eventing library, and if all addon authors used it then the above concerns wouldn't be an issue. If that's not the case, let me know, we can dive into why the libraries that exist are not adequate!

So, addressing each concern individually:

Note: I started writing up this response and it got very long, so bear with me! I'm actually planning on converting this to some documentation, because this is exactly the kind of thing we're looking for in examples of converting to autotracking/tracked properties!

1. Eventing as a Common Pattern

Eventing is used pretty frequently in the ecosystem today. I think this search result from Ember Observer gives us a good overview of this. It's hard to measure exact usage since trigger is a common method name, and eventing is mixed into every object, so many objects could be using eventing, but this search gives us a decent idea of the number of usages, if we assume some false positives from classes that actually implement a method named trigger, and false negatives from events that are not sent on the instance using this. I'm also specifically looking at trigger since it tells us how many usages of custom events are out there, as opposed to consumers of existing events.

So, it's definitely used pretty frequently. The second question I have here is, are there better patterns that can replace those usages? How many of the usages are just addons mimicking Ember's own APIs? How many are using eventing because it is the pattern that makes the most sense?

Out of all the top rated addons in this list (perfect 10 score), I'm not seeing one that I think would necessarily make the most sense using eventing. This is because they all seem to be using eventing to communicate changes of state, presumably sending events so that consumers can update their own derived state based on these changes. I think that in most cases, these can be solved better using autotracking, in a way that is both easier to write and maintain, and less error prone.

I actually love the example you provided, because it's a perfect way to demonstrate this, let's dig in!

Make use of a service to wrap a third party event (very simple example the online event). And then allow other parts of the app to subscribe to that event, so they could display the correct state.

Alright, so let's see how we would implement this example with Evented today:

// services/online.js
export default Service.extend({
  init() {
    this._super(...arguments);

    window.addEventListener('online', () => {
      this.set('isOnline', true);
      this.trigger('isOnlineDidChange');
    });
    window.removeEventListener('offline', () => {
      this.set('isOnline', false);
      this.trigger('isOnlineDidChange');
    });
  }
});

Pretty simple! We just wrap the native events with Ember's own eventing, set the current state inside of the service so things can use that, and also trigger the event in case we ever need to do some more manual updates whenever that state changes, and we don't want to use observers. We might use this service like so:

// components/offline-banner.js
export default Component.extend({
  onlineService: service('online'),

  showBanner: not('onlineService.isOnline'),

  timerId: null,
  currentTime: 60,

  init() {
    this._super(...arguments);

    this.onlineService.on('isOnlineDidChange', () => {
      if (this.onlineService.isOnline) {
        this.stopTimer()
      } else {
        this.startTimer();
      }
    });
  },

  startTimer() {
    this.timerId = setInterval(() => {
      this.set('currentTime', this.currentTime - 1);
    }, 1000)
  },

  stopTimer() {
    cancelInterval(this.timerId);

    this.set('currentTime', 60);
    this.timerId = null;
  },

  willDestroy() {
    this._super(...arguments);

    this.stopTimer();
  }
})
// components/offline-banner.hbs
<div class="offline-banner {{if this.showBanner 'visible'}}">
  You're offline! Reconnecting in {{this.currentTime}} seconds.
</div>

So, we can see in this example that we were able to derive some of the offline state using computed properties, but we can't easily setup or teardown the interval for the timer with a CP. We could use an observer here, but that's not recommended, so this seems like a better pattern overall.

Note: you could also argue that the timer should be put directly on the service, but lets assume it makes more sense on the component, for the sake of the example

Alright, so, how could we solve this problem in a better way using autotracking? The key here is going to be modifiers and helpers:

export default class OnlineService extends Service {
  @tracked isOnline;

  constructor(owner) {
    super(owner);

    window.addEventListener('online', () => this.isOnline = true);
    window.removeEventListener('offline', () => this.isOnline = false);
  }
}
// components/offline-banner.js
export default class OfflineBanner extends Component {
  @service('online') onlineService;
  
  @tracked currentTime = 60;
  
  #timerId = null;

  get showBanner() {
    return !this.onlineService.isOnline;
  }

  @action
  updateTimer() {
    if (this.onlineService.isOnline) {
      this.stopTimer()
    } else {
      this.startTimer();
    }
  }

  startTimer() {
    this.#timerId = setInterval(() => this.currentTime--, 1000)
  }

  stopTimer() {
    cancelInterval(this.#timerId);

    this.currentTime = 60;
    this.#timerId = null;
  }

  willDestroy() {
    super.willDestroy(...arguments);
 
    this.stopTimer();
  }
}
// components/offline-banner.hbs
<div
  {{did-render this.updateTimer}}
  class="offline-banner {{if this.showBanner 'visible'}}"
>
  You're offline! Reconnecting in {{this.currentTime}} seconds.
</div>
// modifiers/did-render.js
import modifier from 'ember-functional-modifiers';

export default modifier(([fn]) => fn());

Alright, so let's break down what's going on here:

  1. We have a did-render modifier, written using ember-functional-modifiers which receives a function and runs that function on insert/update of an element, similar to the didRender hook for classic components.
  2. When that render hook runs for the first time, it autotracks any values that are accessed while running it. If any of those values update, then the modifier's update hook will be called in order to update the state of the modifier. In this case, this calls the same function again.

This effectively gets us the same functionality as before, and without any eventing! We're using the same code that we use for tracking state to track side-effects to.

Why is this any better?

There are two reasons I would say this is better than using an evented pattern:

  1. We're using the same change tracking system as the rest of the codebase, meaning we have one less dependency, and less code overall. This is probably a small amount of code though, so it's not a huge win, but nice anyways.

  2. This method connects side-effects like our setInterval code directly to the state that it needs in order to update. This is a simple example, so there is only one piece of state (isOnline), but there could be many different events and signals that could feed into a single side-effect like this. Rather than manually hooking up each and every single event to our consumers of events, we can bypass this and just access the state when we run our side effect, and rely on the state management system to handle things.

    This is the main benefit of autotracking. We no longer need to manually setup events, manage producers and conusmers, or progagate changes. We can define declarative consumers that access state as needed, and let the system manage itself. In addition, autotracking is able to optimize this much more than any eventing/observable based system would be able to, but that's a bit of a separate topic.

Can we do even better?

This code is pretty similar to the old code, overall. It may read better or worse depending on how you look at it, but it's not a major shift overall. But could we do better? Here's a similar, helper based solution:

export default class OnlineService extends Service {
  @tracked isOnline;

  constructor(owner) {
    super(owner)

    window.addEventListener('online', () => this.isOnline = true);
    window.removeEventListener('offline', () => this.isOnline = false);
  }
}
// components/offline-banner.js
export default class OfflineBanner extends Component {
  @service('online') onlineService;
  
  get showBanner() {
    return !this.onlineService.isOnline;
  }
}
// components/offline-banner.hbs
<div class="offline-banner {{if this.showBanner 'visible'}}">
  You're offline! Reconnecting in {{if this.showBanner (count-down 60)}} seconds.
</div>
// helpers/count-down.js
export default class CountDown extends Helper {
  @tracked currentTime;

  #timerId = null;

  compute(initialTime) {
    if (this.#timerId === null) {
      this.currentTime = initialTime;
      this.#timerId = setInterval(() => this.currentTime--, 1000);
    }

    return this.currentTime;
  }

  willDestroy() {
    super.willDestroy(...arguments);

    clearInterval(this.#timerId);
  }
}

This gives us a new general-purpose utility, a countdown timer, that's completely self-contained and can be used pretty much anywhere. Like with modifiers, the compute() function of a class-based helper is autotracked, so this will recompute whenever the currentTime changes. Setting up and tearing down the interval is now not directly related to the change of state in the service at all - instead, it will change the if statement in the template, which will teardown or setup the helper accordingly.

I think helpers like this will be very helpful in the future, along with side-effecting helpers and such. I'm hoping we can make the dev ergonomics around them better, ideally matching the modifier APIs, but that'll take some time.

Summing Up

So, my belief here based on an initial scan of the ecosystem is that there are quite a few patterns that could be cleaned up using more modern solutions. Another huge set of usages includes exposing addon lifecycle hooks as events, e.g.:

didInsert(component) {
  component.didInsert();
  component.trigger('didInsert');
}

These could also be cleaned up by removing the events altogether, and converting to just using lifecycle hooks.

That said, we definitely should raise as many of the use cases that are unusual or difficult to convert as possible! We did this when making decisions around lifecycle hooks in Glimmer Components, and the audit of the ecosystem and the various types of use cases out there really helped inform that decision, so going through and cataloging the types of use cases, plus generating upgrade guides for existing eventing patterns, would be very valuable in this process.

2. Choosing a Standard Solution

Now, if we assume that there aren't many cases of eventing that remain after we've done the work to convert, upgrade, and remove the ones that can be, then maybe this isn't a huge issue. If only one or two addons out in the ecosystem actually need an eventing layer, then the likelihood of collision is small, and the addons could collaborate to choose a single solution.

It'll be hard to be certain of this though, even with an audit - closed-source code can't be audited, and we probably won't be able to review every single addon in depth.

I think it's totally reasonable to try to choose a recommended eventing library from the existing solutions. I think the most important things are:

  1. Is browser compatible
  2. Is well maintained
  3. Is small and fast

So if we can find one that is the best in all categories, I'm all for it. If there are tradeoffs, this may be a bit difficult though, so something to keep in mind.

Hopefully that wall of text doesn't scare people off, but I actually think this is a great point to dive in on and I'm really glad I got going on it this morning 😄 Thanks again @st-h!

@st-h
Copy link

st-h commented Aug 27, 2019

@pzuraq Thanks a lot for putting this together. I think this addresses all my concerns and in my opinion this would make up a great How we teach this section for the RFC (and of course content for the guides). I am not very familiar with all the details about the octane edition yet - mostly because I was running into issues each time I tried to make use of glimmer components, but didn't have the time to dig any deeper.
As long as we are able to solve those use cases by using core ember features, I don't think we need to consider maintaining a dedicated add-on. I was mainly concerned that we would remove a feature just for the sake of no longer needing it internally, but that does not seem to be the case at all as you pointed out.
And yes, writing code to implement a publisher subscriber pattern can usually be done without much hassle and a few lines of code. I was adding something similar to an app quite a while ago, however the reason was that I needed to bypass the runloop as I needed more frequent updates to keep drawing a canvas smooth.

Gerald Gayowsky and others added 3 commits August 30, 2019 09:58
@lindyhopchris
Copy link

So having given this a lot of thought, I'm totally unsure about this RFC. I use events extensively, because it's such a common design pattern for application development (both backend and frontend) - i.e. for message based systems.

Although I like the example above that uses auto-tracking, and can see how I could use that to replace some of my use of events (and would definitely be better), I'm struggling to see how I can replace every use of events in my applications. The example assumes that you are using events to communicate state changes, but that's not always the case.

As an example, we receive events from the backend server over websockets, that we then re-fire in our Ember app... i.e. so that logically an event on the backend is effectively replicated in the frontend - which makes development simpler because backend and frontend developers work with the same events (even if they are not actually the same events!)

In this scenario our service that receives, processes and fires the events in the Ember app doesn't hold state... i.e. it has no logical conception of what is listening to it, or whether the event is actually going to be used - its job is just to ensure the event that occurred on the backend has an equivalent in the Ember app.

Now I totally accept that there are other event packages that I could choose to use if Ember decides to remove its event system as per this RFC. So in that sense I'm not worried about this RFC.

What does worry me though is event based messaging systems are a common application design pattern. So I see it as totally feasible that addons might not be consistent in choosing the same event based package, meaning we end up with multiple installed in our apps which is not a great position to be in.

Considering Ember is an application framework, I personally think a light-weight event capability should legitimately be provided by the framework. However, I can see the case for it being an optional install...

I.e. I'm proposing that while the framework might want to remove events from core, there should be an official Ember event addon. That could either keep the existing code that is extracted from core, or wrap one of the available npm event packages. Not bothered which, as long as it kept some unity in what you use if your app or addon wants to use an event-based design pattern.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 9, 2019

@lindyhopchris can you provide more concrete examples of the types of events that your backend sends, and that you re-fire on the frontend? This does sound like it may be a a good example of a use case for an eventing pattern, but it would help to make the discussion a bit more concrete.

I'm also curious as to what types of use cases you think would cause addons to be affected. This seems like an application concern, so I don't think most addons would be affected by this particular issue, and I find it difficult to see how this would become an ecosystem problem.

Regarding providing an eventing library, would the core team recommending a specific existing library (especially if one is better than the others), as opposed to creating/maintaining a wrapper, be a good solution to your concerns? Keep in mind, this affect isn't just about the Ember ecosystem - other JS libraries may be using eventing as well, and may actually be more likely too in the future, since Ember is moving away from eventing as a common pattern, but the rest of the ecosystem isn't necessarily.

@lindyhopchris
Copy link

@pzuraq thanks for your response. here's the answers to your questions...

can you provide more concrete examples of the types of events that your backend sends, and that you re-fire on the frontend?

We have multiple pieces of hardware connected to the system that our Ember app is the UI for. The user controls those pieces of hardware, but things can happen independently on the hardware. As one example, a device can be rebooted. So we fire device rebooting and device rebooted events on the backend, that then get broadcast to the front-end, where the events are displayed/notified to the user, plus prevent the user from doing things while those events are happening.

Potentially we could refactor these as each device does have an Ember Data model, so we could wrap up something that keeps track of the model, plus state properties that are changed when we receive the events, etc. But I'm not sure how this is an improvements, as the event-based system works well and accurately reflects what's going on.

I'm also curious as to what types of use cases you think would cause addons to be affected.

One of the addons we use is Ember websockets... that does everything via events on the object that represents the socket. Not sure what the alternative pattern would be for that, and whether an alternative pattern would work?
https://github.com/thoov/ember-websockets

Regarding providing an eventing library, would the core team recommending a specific existing library (especially if one is better than the others), as opposed to creating/maintaining a wrapper, be a good solution to your concerns?

Yes, I suppose that would cover it. I suppose I'm raising my concerns not in a please don't do this! way, but more a if you're going to do this, I feel there still needs to be an Ember-way of doing events. Hope that makes sense?

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Sep 9, 2019

Fwiw, I have a websockets powered app, https://emberclear.io, and I don't use events.

I'm explicitly calling functions defined on other services.

This is how I handle incoming messages: https://github.com/NullVoxPopuli/emberclear/blob/master/packages/frontend/app/services/messages/handler.ts#L22

Later on, it's gonna be tied up to a few xstate machines as well

@lindyhopchris
Copy link

I'm explicitly calling functions defined on other services.

Hmm, yeah I can see that works but I wouldn't normally architect an event-message system like that, because it gives one service too much explicit knowledge of the other services, rather than decoupling them. To be clear, not saying it's wrong as a lot of these approaches are down to developer preference - just saying it wouldn't be my choice. (I.e. please don't interpret this comment as a criticism!)

Anyway, I think I've probably said everything I wanted to to input into this discussion. Interested to see what the decision on this RFC will be!

@pzuraq
Copy link
Contributor

pzuraq commented Sep 9, 2019

One of the addons we use is Ember websockets... that does everything via events on the object that represents the socket.

Ah, I think maybe I should clarify here, it's clear that certain data patterns would require eventing, for instance if you use websockets in this way. If you use Apollo, it has it's own Observable based eventing, so that would be another example where a data solution would bring eventing back into an app.

What's not clear is whether or not this would be a common pattern throughout the ecosystem. Would addons that provide components be affected by this? Would addons that provide authentication/authorization, such as ember-simple-auth, be affected? Are there addons that provide additional functionality on top of ember-websockets that would be affected?

Genuine questions here. Basically, if it's unlikely that an application will include multiple eventing libraries in the first place, because there isn't much overlap in the types of things that require eventing, then I think there is less of a concern than if they do, if that makes sense.

if you're going to do this, I feel there still needs to be an Ember-way of doing events. Hope that makes sense?

Definitely makes sense, I think ideally we would provide some kind of guidance here, for sure 😄 thanks for clarifying btw, really appreciate it, this helps a lot!

@NullVoxPopuli
Copy link
Contributor

Would rx.js with a test waiters integration be a better path forward? I wonder if that'd be code moddable somehow...

@NullVoxPopuli
Copy link
Contributor

Reason why I bring up rx.js, is that it's a huuuuuge community-wide used library, and super lightweight and solid for event/observable patterns

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I'm pretty sure this is something we need to do. I'll bring this up with the core team and see if we can't get this finalized.

@wagenet
Copy link
Member

wagenet commented Jul 29, 2022

I believe this falls under the umbrella of #832. I'm going to close this issue to push any discussion there. Despite being closed, we will certainly use this issue for reference and to inform the future RFC. Thank you all for the discussion!

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.