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] Refactor DataAdapter to not use observers or array observers #19379

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 4, 2021

The DataAdapter class is exposed specifically for the Ember Inspector to
support Ember Data. Currently, the adapter relies on both array
observers and standard observers. This PR refactors the adapter to use
autotracking-based methods for tracking changes instead, with minimal
API churn.

The main change is to introduce a regular poll that checks for changes
after each backburner runloop. There are two types of polling done:

  1. TypeWatcher, which checks to see if a record array has changed at
    all (e.g. contents have changed)
  2. RecordsWatcher, which watches the array as a whole and the
    individual records within it. Whenever any changes occur, it
    reiterates the records array, and looks for new records, updated
    records, and removed records, and notifies of any changes.

This removes the need for both array observers and standard observers.
Standard observers were previously used to detect changes on the model
instances, but now we autotrack serializing them, and that
autotracking is used to detect if they've been updated.

## Breaking Changes

  • The recordsRemoved callback from watchRecords has changed. It now
    receives an array of removed records, not an index and count. On the
    consuming side in Ember Inspector, we'll have to update the code to
    remove the items passed via id explicitly.
  • The observeRecord method has been removed. Because serializing the
    object is observing it (via autotracking) there is no need to
    manually observe it. Users who are still implementing this method
    (e.g. Ember Data) can still keep it, and it shouldn't cause issues, it
    just will no longer be called.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I left a few inline comments, but we also need to change things around to avoid adding more than one backburner.on('end', ...) callback as well as ensuring that we clean it up upon destruction. As it stands now, we leak in a few ways (registering the same callback many times, never removing but adding another callback per-container, etc).

@@ -115,9 +115,9 @@ module.exports = {
'cacheFor',
'camelize',
'canCatalogEntriesByType',
'canInvoke',
'cancel',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was partially sorted, but not fully. I decided to sort it to make it consistent.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Still doesn't seem to backburner.off('end', this.flushWatchers) upon destruction (which means it leaks across multiple owners / tests).

The DataAdapter class is exposed specifically for the Ember Inspector to
support Ember Data. Currently, the adapter relies on both array
observers and standard observers. This PR refactors the adapter to use
autotracking-based methods for tracking changes instead, with minimal
API churn.

The main change is to introduce a regular poll that checks for changes
after each backburner runloop. There are two types of polling done:

1. `TypeWatcher`, which checks to see if a record array has changed at
   all (e.g. contents have changed)
2. `RecordsWatcher`, which watches the array as a whole _and_ the
   individual records within it. Whenever any changes occur, it
   reiterates the records array, and looks for new records, updated
   records, and removed records, and notifies of any changes.

This removes the need for both array observers _and_ standard observers.
Standard observers were previously used to detect changes on the model
instances, but now we autotrack _serializing_ them, and that
autotracking is used to detect if they've been updated.

\## Breaking Changes

- The `recordsRemoved` callback from `watchRecords` has changed. It now
  receives an array of removed records, _not_ an index and count. On the
  consuming side in Ember Inspector, we'll have to update the code to
  remove the items passed via id explicitly.
- The `observeRecord` method has been removed. Because serializing the
  object _is_ observing it (via autotracking) there is no need to
  manually observe it. Users who are still implementing this method
  (e.g. Ember Data) can still keep it, and it shouldn't cause issues, it
  just will no longer be called.
@rwjblue rwjblue merged commit e856dd2 into master Feb 5, 2021
@rwjblue rwjblue deleted the refactor-data-adapter branch February 5, 2021 21:41
@snewcomer
Copy link
Contributor

https://github.com/emberjs/data/pull/7435/files

We have a minor test issue as a result of this refactor. Any ideas on how we can wait for the flush to happen? I see you are taking advantage of runLoopSettled in the tests here...

@pzuraq
Copy link
Contributor Author

pzuraq commented Feb 20, 2021

Correct, you should be able to use the settled() helper from @ember/test-helpers, which is essentially the same. I believe I tested this out locally before we merged this change

@snewcomer
Copy link
Contributor

Oh jeez. I was putting the settled() after the async action but before set. Now it works with it in the right place. Thanks!

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.

None yet

3 participants