Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[CLEANUP beta] Remove beforeObserver family from the public API#11796

Merged
stefanpenner merged 1 commit intoemberjs:masterfrom
cibernox:remove_before_observers
Jul 17, 2015
Merged

[CLEANUP beta] Remove beforeObserver family from the public API#11796
stefanpenner merged 1 commit intoemberjs:masterfrom
cibernox:remove_before_observers

Conversation

Copy link
Contributor

cibernox commented Jul 17, 2015

  • family inclides Ember.beforeObserver, Ember.addBeforeObserver,
    Ember.removeBeforeObserver, Ember.beforeObserversFor,
    Ember._suspendBeforeObserver, Ember._suspendBeforeObservers
  • I noticed that beforeObserversFor, _suspendBeforeObserver and _suspendBeforeObserves
    were not used at all, not even internally. I've also checked for
    usages of it in ember-data, liquid-fire and other "official" porjects.
    Nothing. So those two are removed completely.
  • For symetry, I've also removed the Function.prototype.observesBefore
    counterpart.

Copy link
Member

stefanpenner Jul 17, 2015

Choose a reason for hiding this comment

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

this appears to have have been deprecated.

Copy link
Member

rwjblue Jul 17, 2015

Choose a reason for hiding this comment

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

Do you mean, it was not deprecated?

Copy link
Member

stefanpenner Jul 17, 2015

Choose a reason for hiding this comment

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

Sorry, I mean it did no spew a deprecation warning when used. Is there a sibling PR that introduces that deprecation.

Copy link
Member

rwjblue Jul 17, 2015

Choose a reason for hiding this comment

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

Ya, good catch.

@cibernox - We need to land a deprecation for this in release before removing.

Copy link
Contributor Author

cibernox Jul 17, 2015

Choose a reason for hiding this comment

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

There isn't. I'll create one. Put this on hold in the meanwhile

Copy link
Contributor Author

cibernox Jul 17, 2015

Choose a reason for hiding this comment

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

Copy link
Member

rwjblue commented Jul 17, 2015

Needs a rebase now that #11798 is merged.

* family inclides `Ember.beforeObserver`, `Ember.addBeforeObserver`,
`Ember.removeBeforeObserver`, `Ember.beforeObserversFor`,
`Ember._suspendBeforeObserver`, `Ember._suspendBeforeObservers`

* I noticed that `beforeObserversFor`, `_suspendBeforeObserver` and `_suspendBeforeObserves`
were not used at all, not even internally. I've also checked for
usages of it in ember-data, liquid-fire and other "official" porjects.
Nothing. So those two are removed completely.

* For symetry, I've also removed the `Function.prototype.observesBefore`
counterpart.
cibernox force-pushed the remove_before_observers branch from 3011b73 to 75569a9 Compare July 17, 2015 19:30
Copy link
Contributor Author

cibernox commented Jul 17, 2015

Rebased

stefanpenner added a commit that referenced this pull request Jul 17, 2015
[CLEANUP beta] Remove beforeObserver family from the public API
stefanpenner merged commit 3ab4857 into emberjs:master Jul 17, 2015
cibernox deleted the remove_before_observers branch July 17, 2015 19:58
Copy link

denzo commented Aug 15, 2015

SortableMixin from ember-legacy-controllers breaks as it uses beforeObserver :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants