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

[CLEANUP beta] Remove deprecated array observers #19833

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

kategengler
Copy link
Member

@kategengler kategengler commented Nov 12, 2021

Part of #19617

Supplants #19746

Co-authored-by: Matthew Beale [email protected]

@mixonic mixonic force-pushed the kg-remove-addarrayobserver branch 9 times, most recently from a5f28d2 to 2430326 Compare November 12, 2021 03:31
@mixonic mixonic changed the title Remove deprecated addArrayObserver and removeArrayObserver [CLEANUP beta] Remove deprecated addArrayObserver and removeArrayObserver Nov 12, 2021
@mixonic mixonic force-pushed the kg-remove-addarrayobserver branch from 2430326 to 825bf5f Compare November 12, 2021 15:17
* Array proxies have a `_revalidate` method which must be called to set
* up their internal array observation systems.
*/
obj._revalidate?.();
Copy link
Member

Choose a reason for hiding this comment

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

In this patch hasArrayObserver has been removed. That property was not directly deprecated in 3.x, however we've removed a number of APIs in this patch which were related to someArray.addArrayObserver (APIs which were only meaningful once you had an observer installed).

Because it is removed, it no longer needs to be notified of change. However the _revalidate hook on ArrayProxy instances was being called via PROPERTY_DID_CHANGE symbol notification. Without trying to rewrite that system we have two options: Either call _revalidate() directly, or keep the property hasArrayObserver around just to trigger the PROPERTY_DID_CHANGE. Here I've chosen to call _revalidate() directly.

@mixonic mixonic force-pushed the kg-remove-addarrayobserver branch 2 times, most recently from ebcf3a1 to d374744 Compare November 12, 2021 15:37
Part of emberjs#19617

* Remove `someArray.addArrayObserver`
  https://github.com/emberjs/ember.js/pull/19833/files#diff-8116921433c6e1664250d5735e4f9daeb6ea9e00940783e0c4db7adf88aa8772L532
* Remove `someArray.removeArrayObserver`
  https://github.com/emberjs/ember.js/pull/19833/files#diff-8116921433c6e1664250d5735e4f9daeb6ea9e00940783e0c4db7adf88aa8772L548
* Remove `someArray.hasArrayObservers`
  https://github.com/emberjs/ember.js/pull/19833/files#diff-8116921433c6e1664250d5735e4f9daeb6ea9e00940783e0c4db7adf88aa8772L571
* Remove `someArray.arrayContentWillChange`
  https://github.com/emberjs/ember.js/pull/19833/files#diff-8116921433c6e1664250d5735e4f9daeb6ea9e00940783e0c4db7adf88aa8772L642
* Remove `someArray.arrayContentDidChange`
  https://github.com/emberjs/ember.js/pull/19833/files#diff-8116921433c6e1664250d5735e4f9daeb6ea9e00940783e0c4db7adf88aa8772L670
  (also at
  https://github.com/emberjs/ember.js/pull/19833/files#diff-79a0015ca0e6a6c3661d9119f4caf6b26b111af6bb0e5bb784558631c599a81cL357)
* On the internal version of `addArrayObserver`, remove the optionality
  of `willChange` and `didChange` arguments to name the hooks. In
  framework code (and now in test) these are always provided.
* Retain basically all the test coverage of array observers since they
  remain used by `ArrayProxy` internally (and through that interface, in
  Ember Data).

Co-authored-by: Matthew Beale <[email protected]>
@mixonic mixonic changed the title [CLEANUP beta] Remove deprecated addArrayObserver and removeArrayObserver [CLEANUP beta] Remove deprecated array observers Nov 12, 2021
@mixonic mixonic force-pushed the kg-remove-addarrayobserver branch from d374744 to a9aedea Compare November 12, 2021 15:45
@mixonic mixonic marked this pull request as ready for review November 12, 2021 15:51
@mixonic mixonic merged commit d612b18 into emberjs:master Nov 12, 2021
@mixonic mixonic mentioned this pull request Nov 12, 2021
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants