Add Ember Data 2.0 Reload behavior

[broerse]

This fixes the #79 deprecation warning and preserves the current behavior until we know the new Reload behavior works and is faster in Ember Data 2.0

[fsmanuel]

👍

[rsutphin]

@broerse, thanks for taking the time to submit this.

There's more detail in my comment on #79, but briefy: I don't think this is a good idea. If I understand things correctly, shouldReloadRecord and shouldBackgroundReloadRecord have no benefits over our existing change watcher. They will make your app do extra work for no gain. If anything, we should default them to false.

I'm less sure about shouldReloadAll and shouldBackgroundReloadAll. There are pluses and minuses to either approach, so I feel like inheriting the ember-data behavior (whatever it may be) is the best thing to do. The adapter will behave the way the default adapters are documented to behave. If folks want to change that behavior, they will have that option.

[broerse]

@rsutphin @fsmanuel I don't think I fully agree but I am fine with closing this.

[fsmanuel]

What about adding shouldReloadRecord: function() { return false; } to remove the deprecation warning. I haven't checked ember-data code to verify that the deprecation will actually go away...

[broerse]

@fsmanuel If that removes the deprecation warning (not checked yet) we shout do all 4 because I get shouldReloadAll deprecation warnings in my apps.

[rsutphin]

we shout do all 4 because I get shouldReloadAll deprecation warnings in my apps.

We should only do all four if we think there's a reason why ember-pouch should specify a value for each one. If not, it should be left up to the person using ember-pouch in an application.

It's not the responsibility of this library to get rid of all deprecation warnings — if you were using a built-in adapter (e.g., RESTAdapter), you'd be seeing the same deprecation warning. It's communicating an upcoming behavior change that you, as an application developer, need to be aware of. It's not the same as the warning in #72 (e.g.) which communicates changes to the structure of the adapter API and so is the responsibility of the library.

[rsutphin]

What about adding shouldReloadRecord: function() { return false; } to remove the deprecation warning.

Deprecation warnings are emitted for shouldReloadAll and shouldBackgroundReloadRecord only:

https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/adapter.js#L452-L529

I would be fine with having ember-pouch override shouldBackgroundReloadRecord to be false, since background reloading of individual records is not necessary in ember-pouch. (shouldReloadRecord is already false and apparently will continue to be false in 2.0.)

For the reasons I've outlined above and elsewhere, I don't think that ember-pouch should make a decision about shouldReloadAll. It should leave the warning in place and let individual application developers decide what to do.

[fsmanuel]

Sounds reasonable!

Am 21.07.2015 um 15:52 schrieb Rhett Sutphin notifications@github.com:

What about adding shouldReloadRecord: function() { return false; } to remove the deprecation warning.

Deprecation warnings are emitted for shouldReloadAll and shouldBackgroundReloadRecord only:

https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/adapter.js#L452-L529

I would be fine with having ember-pouch override shouldBackgroundReloadRecord to be false, since background reloading of individual records is not necessary in ember-pouch. (shouldReloadRecord is already false and apparently will continue to be false in 2.0.)

For the reasons I've outlined above and elsewhere, I don't think that ember-pouch should make a decision about shouldReloadAll. It should leave the warning in place and let individual application developers decide what to do.

—
Reply to this email directly or view it on GitHub.

[broerse]

Just checked. Return false also disables the deprecation warnings but it does not preserve the current behavior. I still think that until we know how the new Reload behavior works we should merge this. It gives us some time to test things out. If I am on my own here I am fine with closing this.

Edit: I just found out that return false on all four stops Ember Data from fetching any data. Perhaps we should return the parent or something. Anyway it strengthened me in the thought we should preserve the current behavior for now.

[rsutphin]

Return false also disables the deprecation warnings but it does not preserve the current behavior.

@broerse, what I'm hearing from you is that you want to preserve the existing ember 1.x behavior. Regardless of whether we agree about that, this pull request does not preserve the existing behavior. The existing behavior is what DS.Adapter in 1.13 does:

• shouldReloadRecord => false
• shouldReloadAll => true

I'm going to close this because I think the consensus is that we will override the single-record methods to return false and continue to do so in ember-data 2.0 — the change watcher means that ember-pouch never needs single records reloaded. We will leave the default behavior for record arrays because there's not a clear correct behavior based on the adapter alone — it needs to be decided by each application developer and so keeping the ember-data default behavior is the least confusing path.

If you disagree with either of those points, please comment.