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

Start RFC to deprecate the use of Ember Evented in Ember Data #329

Merged
merged 1 commit into from Sep 26, 2018

Conversation

@bmac
Copy link
Member

commented May 1, 2018

Rendered

@bmac bmac added the T-ember-data label May 1, 2018

@bmac bmac force-pushed the deprecate-ember-evented-in-ember-data branch 4 times, most recently from c43194d to 5a1e8cd May 1, 2018

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Awesome, I'm definitely in favor of this!

Some smallish things that I think we should clarify in the RFC are:

  • Exact version where evented support will be dropped (I honestly am not sure if its usage is currently public API)
  • Update of the "how do we teach" this section to include reference to updating the deprecation guide app with examples to teach folks how to migrate away from each of the common scenarios

@bmac bmac force-pushed the deprecate-ember-evented-in-ember-data branch from 5a1e8cd to b7e7fce May 1, 2018

@dwickern

This comment has been minimized.

Copy link

commented May 1, 2018

ember-data-change-tracker depends on these events (source). Will there be an alternative?

@btecu

This comment has been minimized.

Copy link

commented May 1, 2018

Some of the events (didLoad) are better than computed (no overhead).

@joukevandermaas

This comment has been minimized.

Copy link

commented May 2, 2018

The motivation section does not mention any reason for removing lifecycle hooks like didLoad. These can definitely not be replaced by a computed in all scenarios. It's not obvious to me why they have to go or what the migration path will be.

@gossi

This comment has been minimized.

Copy link

commented May 2, 2018

If the idea behind this RFC is to strive towards ES classes rsp. allowing a better "programming model" to narrow the gap to traditional OOP-related programming languages and their design patterns then I'd say DS.Model would have blank lifecycle methods of protected visibility state and would allow specific models to overwrite those methods for their particular use case.

Removing events in favor of overwritable methods sounds good to me as this is what happened with components afair.

@runspired

This comment has been minimized.

Copy link
Contributor

commented May 2, 2018

@dwickern there is already a path forward for ember-data-change-tracker with the ModelData RFC.

@btecu using the didLoad event is far from "no overhead". Computeds are much cheaper.

@joukevandermaas Can you provide an example use of didLoad that computeds do not handle?

@gossi lifecycle methods have been debated before for models; however, for the most part there isn't a clear conceptual model for what the lifecycle of a model even is. That said, the ModelData RFC allows for alternative model layers to experiment with this in their own ways.

@joukevandermaas

This comment has been minimized.

Copy link

commented May 3, 2018

@runspired anything that has side-effects really, which is the reason to use lifecycle hooks generally. For example, kicking off a long-running task. Are you saying there is overhead to didLoad even if people don't use it? How was that solved for components in Ember itself?

@runspired

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

@joukevandermaas

anything that has side-effects really, which is the reason to use lifecycle hooks generally.

Side-effect driven development isn't something we seek to support. Kicking off additional work from the promise returned when fetching a record would achieve the same thing in a non-side-effect way. If you seek to centralize this pattern then fetching via a service that has these known additional behaviors would provide a clear abstraction.

Are you saying there is overhead to didLoad even if people don't use it?

Yes. Evented accounts for somewhere between 10 and 30% of our cost to instantiate a record and an even higher percentage of the cost when mutating a record, this even after we did things to make it substantially faster around version 2.10.

How was that solved for components in Ember itself?

At a superficial level, the answer for this difference is that Components have a clear lifecyle, which makes certain patterns easy to optimize. Models do not. At a deeper level the specific issue is that in order to support these events we had to add significant complex anti-patterns throughout the internals. This has made the internals difficult for even regular contributors to understand, and prevented many needed optimizations from taking place.

@joukevandermaas

This comment has been minimized.

Copy link

commented May 3, 2018

@runspired thanks for the clear answer 👍 this makes sense to me. As long as there is a clear migration path I'm OK with anything.

@mehulkar

This comment has been minimized.

Copy link

commented Jul 19, 2018

Could the deprecation contain the message that if the user wants to retain this behavior they can mixin Ember.Evented into their model definition manually; i.e. DS.Model.extend(Ember.Evented, {})? Would this be equivalent to the way things are now?

@runspired

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

@mehulkar possibly; however, this is primarily for events that we trigger.

@sandstrom

This comment has been minimized.

Copy link

commented Aug 15, 2018

@bmac I'm in favour! 👍

@locks locks added this to Deprecations in Deprecation Candidates Sep 18, 2018

@locks locks moved this from Deprecations to In Progress in Deprecation Candidates Sep 18, 2018

@locks locks added the T-deprecation label Sep 18, 2018

@hjdivad

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Discussed during the call today and we're happy to merge 👍

The only change from the current text is that the deprecations will last through an LTS cycle.

@hjdivad hjdivad merged commit 2eea27f into master Sep 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Deprecation Candidates automation moved this from In Progress to Finished Sep 26, 2018

@rwjblue rwjblue deleted the deprecate-ember-evented-in-ember-data branch Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.