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

reload() and fetchById don't restore changed belongsTo #3089

Closed
artych opened this issue May 22, 2015 · 12 comments
Closed

reload() and fetchById don't restore changed belongsTo #3089

artych opened this issue May 22, 2015 · 12 comments

Comments

@artych
Copy link

artych commented May 22, 2015

It is known that if you change record's belongsTo property without saving, record doesn't become dirty and can't be restored by rollback() emberjs/rfcs#21.

But it was unexpected for me that it also can't be restored by reload(), and can't be reloaded by store.fetchById(). The only way works for me is record.unloadRecord() and then store.find(..). See http://jsbin.com/qoyacagigi/edit?html,js,output.

Feel free to close if this is expected behavior.

@pangratz
Copy link
Member

The related test is unit/model/merge-test which checks that locally modified attributes are not reset after a merge. The test (currently) doesn't have assertions for relationships, so I guess the correct behavior is yet to be decided. I definitely think it should be the same as for attributes, so locally changed relationships should not be reset after a reload.

@artych
Copy link
Author

artych commented May 22, 2015

@pangratz, thanks. And what about fetchById? http://emberjs.com/api/data/classes/DS.Store.html#method_fetchById. I expected it to be "hard reset version" of find method (that fetch model from backend and then create/override model in store).

@pangratz
Copy link
Member

According to the doc fetchById only calls reload so the same semantics apply.

Restoring local relationship changes via rollback should eventually work; it is on the meta list for 1.0 (first entry).

@artych
Copy link
Author

artych commented May 22, 2015

I mean is my expectation about fetchById wrong or not?
Looks like I am not alone, for example see last 2 comments in http://discuss.emberjs.com/t/force-store-to-reload-data-from-api/4441/12.

@pangratz
Copy link
Member

I mean is my expectation about fetchById wrong or not?

Your confusion is justified as the naming of the methods is not consistent at the moment. There is an open RFC which discusses the semantics of the various methods to get a consistent naming.

@artych
Copy link
Author

artych commented May 22, 2015

@pangratz, thanks for link.
After reading emberjs/rfcs#27 it is clear that fetchById is expected to load data from the server, even if it's already in the local store.
Therefore I think this is valid issue in case findById fetchById method.

@artych
Copy link
Author

artych commented May 22, 2015

about reload(): "locally modified attributes are not reset after a merge..."

as for me reload() is very confusing name for such a behavior
and no description about it here: http://emberjs.com/api/data/classes/DS.Model.html#method_reload

@pangratz
Copy link
Member

That method documentation is highly outdated and has been last updated at 22. April 2013 😮 Want to submit a PR?

@artych
Copy link
Author

artych commented May 22, 2015

I'd like to, but it is not clearly for me what such a behavior for, so I do not know what description should be written. May be somebody can explain me...

@artych
Copy link
Author

artych commented May 22, 2015

I would like to propose PR with changes:

  • for reload() method add optional parameter to specify merge strategy.

For example: force: boolean, defaultValue: false, so model.reload() and model.reload(false) behaves as is now, but model.reload(true) loads record from server with "hard reset" merging strategy (ignoring locally modified attributes and associations).

  • in store.fetchById(...) source code changereload() for reload(true)
  • documentation fixing.

Does it look reasonable?

@artych
Copy link
Author

artych commented May 23, 2015

As an alternative just to implement new fetching method with "hard reset" merging strategy. Something like this:

    fetchOriginById: function(modelName, id, preload) {
        if (this.hasRecordForId(modelName, id)) {
            this.getById(modelName, id).unloadRecord();
        }
        return this.find(modelName, id, preload);
    }

UPD: this code fails with #3084 issue.

@artych
Copy link
Author

artych commented May 29, 2015

closed since no confirmation

@artych artych closed this as completed May 29, 2015
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

No branches or pull requests

2 participants