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

Intent to RFC: Deprecate Observers #533

Closed
pzuraq opened this issue Aug 30, 2019 · 17 comments
Closed

Intent to RFC: Deprecate Observers #533

pzuraq opened this issue Aug 30, 2019 · 17 comments
Labels
Seeking Co-author T-deprecation T-framework RFCs that impact the ember.js library

Comments

@pzuraq
Copy link
Contributor

pzuraq commented Aug 30, 2019

  • Proposed Target: Ember v4.0.0
  • Alternative: Autotracking + tracked properties

Observers have been considered an antipattern in Ember for some time now, and with autotracking currently being implemented, all of the use cases for observers now have better, more idiomatic alternatives. This pre-RFC proposes that we remove them from the framework in the near future, and focus on helping existing users migrate to autotracking based solutions.

Migration Path

Observers → autotracking is not always a simple 1-1 conversion, sometimes it does mean there is a decent amount of refactoring that needs to occur. We should develop a number of examples for every use case that arises throughout the conversion process, and try to store these examples in a shared community space such as the official guides or the Ember Atlas.

@locks locks added T-deprecation T-framework RFCs that impact the ember.js library labels Sep 2, 2019
@pzuraq pzuraq changed the title Pre-RFC: Deprecate Observers Intent to RFC: Deprecate Observers Oct 8, 2019
@jede
Copy link

jede commented Apr 2, 2020

I hope I'm not hijacking this issue. We are struggling with how to transition out from observers in our Ember app. I thought I would share an example here of what our challanges are, since you are looking to build up examples of use cases.

For this case we have a 3rd party WebRTC library that starts when we call startStream. We cannot start the stream before status is 'live' and isConnected is true After refactoring away observers this is what we came up with:

  _status: null,
  status: computed({
    set(key, value) {
      const status = value.value
      this.set('_status', status)
      const isConnected = this.get('isConnected')
      this.handleStream(isConnected, status)
      return status
    },
    get() {
      return this.get('_status')
    }
  }),

  _isConnected: false,
  isConnected: computed({
    set(key, value) {
      this.set('_isConnected', value)
      const status = this.get('status')
      this.handleStream(value, status)
      return value
    },
    get() {
      return this.get('_isConnected')
    }
  }),

  handleStream(isConnected, status) {
    if (!isConnected) return
    if (status === 'live') {
      this.startStream()
    } 
  }

The old way:

handleStream: observer('liveStream.status', 'isConnected', function() {
    if (!this.get('isConnected')) return
    if (this.get('status') === 'live') {
      this.startStream()
    } 
  })

The old way with an observer just looks so much shorter and easier to follow. I think this is a good use case for an observer that should really be addressed with an example to make it easier to move forward. Are we missing some better way of doing this?

Thanks!

@Gaurav0
Copy link
Contributor

Gaurav0 commented Apr 2, 2020

There are lots of apps out there with lots of observers that cannot be easily removed.

I don't think we should yet be discussing deprecating them "in the near future". We need to better discuss the process for moving things forward for things this deeply embedded in our framework.

@chriskrycho
Copy link
Contributor

@jede I'd suggest moving that question to discuss.emberjs.com – the forum is probably the better context for that kind of open-ended discussion!

@jede
Copy link

jede commented Apr 2, 2020

@chriskrycho thanks, thats probably a good idea. However I think the discussion around deprecating observers really need more concrete examples, Iike described in this RFC intent. So my idea of posting it here is to discuss how Ember is supposed to solve a situation like this. Just highlighting a simple, but yet frustrating, real world example of how observers are used and were I at least struggle to see how deprecating observers all together is a good idea. But maybe I’m missing something really obvious?

@jrjohnson
Copy link

I agree with @jede examples like this belong here, but putting the solutions here is going to be distracting. I'd suggest that maybe a question on stack overflow and a link here? So there is a record of the specific use case as well as an opportunity to community source better patterns which will eventually end up in the RFC.

@chriskrycho
Copy link
Contributor

chriskrycho commented Apr 2, 2020

@jede @jrjohnson didn't mean to imply good examples weren't valuable, so I apologize that it read that way (and I see how it did)! I think there are a couple good alternative ways to solve that particular problem, I just didn't want to clutter this thread with it! (Spoilers: a modifier solves this extremely nicely!)

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 2, 2020

@jede can you provide the remainder of the code you've in your example? Specifically, startStream - how does it work, and how do users interact with the output?

I ask because I believe the solution here would be to use Resources. Resources, however, are syntactic sugar on top of functionality that is for the most part possible to accomplish today with autotracking. I'm planning on doing some deep dives in the near future as part of the continuation of my Autotracking blog post series to show step by step how to do this, and this seems like it would be a perfect example 😄

You could of course continue to side-effect using autotracking, but ideally we would turn the stream into a first class data structure, similar to the way Ember Concurrency did this with fetch requests, which is why I ask for the remainder of the example.

@jede
Copy link

jede commented Apr 3, 2020

@pzuraq first of all, thanks for looking into this! I left out the remainder of the startStream method because it's all 3rd party that inserts its own DOM. I agree with you that ideally we would be able to turn it into a first class data structure, but we don't control the library unfortunately. So that's where it gets tricky I guess.

Resources looks like an interesting approach, but I'm still not sure how you would trigger the resource to start the live stream in this case where we depend on multiple properties?

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 3, 2020

Maybe it's not clear in the RFC, but Resources can use and respond to changes in any number of arguments:

@use myStream = stream(this.args.status, this.args.isConnected);

You can think of them in a way like Helpers that you can declare in JavaScript. They receive arguments, and produce a value (in this case, a stream which would expose some sort of data, ideally).

@jede
Copy link

jede commented Apr 3, 2020

@chriskrycho I put the question on the forum as well https://discuss.emberjs.com/t/move-away-from-observers/17731 :)

(Spoilers: a modifier solves this extremely nicely!)

Thanks for the spoiler! Would you care to elaborate? Here or on the forum? I don't understand how you would implement a modifier that solves this (waiting for two properties to have desired values). I'm not familiar with the modifier concept from earlier (I read the RFC now), but from what I understand you would just move the same problem to the modifier instead?

@jede
Copy link

jede commented Apr 3, 2020

@pzuraq So basically the Resource would be invoked every time the arguments changed? That would solve our case case nicely I think!

We would not be returning any data however (we don't really get any data back from the library at that stage, we just subscribe to a couple of callbacks and calls it a day).

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 3, 2020

@jede well, the structure is moreso that you would have to use the resource actively, from a template for instance, for it to be called again:

{{this.myStream.data}}

The resource could of course be defined entirely within a template too, with the current proposal it would look like:

{{use stream this.args.status, this.args.isConsumed}}

And then it could return undefined or something, and just not show up at all. This is why we would want to focus on making it a consumable though, because it is much easier to reason about than side-effecting in general.

If all you want to do though is side-effect, we are thinking that something like that could exist too:

constructor() {
  use(this, effect(() => {
    if (!this.args.isConnected) return;
    if (this.args.status === 'live') {
      this.startStream()
    } 
  });
}

This would also autotrack and would rerun whenever any value inside of it changed, but with one key distinction - it would not allow any updates to any tracked state (or computeds, using Ember.set, etc) in the application. The idea is that effects would be for reflecting state out of Ember, not for reflecting state within it. Inside an Ember app, using this pattern has many of the same general downsides that observers had - it makes it difficult to follow, order of operations is not clear, etc. But there are times when we need to interact with non-autotracked external systems, and this is what effect would be for.

If all you're doing is registering a couple callbacks, I think effect might work for that here.

@chriskrycho
Copy link
Contributor

chriskrycho commented Apr 3, 2020

@jede as well as liking @pzuraq's suggestion of a resource-based approach, I wrote up a modifiers-based approach on the forum – it's possible I'm missing something, but I believe that would correctly handle the same things your observer handles.

To answer this question:

from what I understand you would just move the same problem to the modifier instead

The difference is that modifiers participate natively in auto-tracking's pull-based system, rather than observers' push-based system. This is the same insight which makes the resource APIs work: it's a side-effect-friendly part of the system—an 'escape hatch' of sorts—which works via the auto-tracking APIs.


My current take, on the overall subject of this thread, is that observers can all be replaced – usually fairly straightforwardly – by one of the full set of Octane features. The key is determining which one, and thinking through how. That complexity should (and will!) inform when and how observers are deprecated. Along the way, we'll need to build up a community-wide set of resources explaining the patterns for migration: when should you reach for tracked properties and derived state? for modifiers? for something like resources? That should help enormously as it will provide paths for people to pave. We've already started some of that—see for example this fairly complicated example combining mixins, observers, and actions that I wrote up last year. We need a lot more here!

@sandstrom
Copy link
Contributor

sandstrom commented Jun 26, 2020

Just wanted to chime in that for our code base, the ~15 services/components/etc where we use observers could all be migrated to @use + Resource (#567), if that RFC would be implemented.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

We should definitely deprecate these. Let's fast-track an RFC draft.

@chriskrycho
Copy link
Contributor

Probably goes under #832. If need be (depending on how that goes) we can tackle it separately!

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

I’ll close for now and we can reopen if beneficial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Seeking Co-author T-deprecation T-framework RFCs that impact the ember.js library
Projects
None yet
Development

No branches or pull requests

8 participants