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

WIP client-side-delete semantics `unloadRecord` #5273

Merged
merged 2 commits into from Jan 11, 2018

Conversation

Projects
None yet
9 participants
@hjdivad
Copy link
Member

hjdivad commented Dec 6, 2017

Fix #5136, #5137

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Dec 6, 2017

Follow-on for #5136 #5137

@hjdivad hjdivad force-pushed the hjdivad/restore-unloadRecord-client-side-delete-semantics branch from 61cee15 to ed208d0 Dec 14, 2017

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Dec 14, 2017

cc @igorT

@@ -1488,3 +1488,8 @@ test('many:many async unload', function (assert) {
assert.equal(person1Friends.isDestroyed, true, 'previous ManyArray is destroyed in the runloop after refetching');
});
});

test('1 sync : 1 async');

This comment has been minimized.

@rwjblue

rwjblue Dec 18, 2017

Member

Fwiw I do t think QUnit supports this syntax (assuming you are intending to basically create a skip?)...

This comment has been minimized.

@hjdivad

hjdivad Jan 3, 2018

Member

@rwjblue creating a todo, not a skip.

@hjdivad hjdivad force-pushed the hjdivad/restore-unloadRecord-client-side-delete-semantics branch from 87c8129 to aa59926 Jan 4, 2018

this.__loadingPromise = null;
}

_inverseIsAsync() {

This comment has been minimized.

@igorT

igorT Jan 9, 2018

Member

This could live on the parent class

@@ -44,6 +52,10 @@ export default class ManyRelationship extends Relationship {
isPolymorphic: this.isPolymorphic
});
}
if (this._retainedManyArray !== null) {

This comment has been minimized.

@igorT

igorT Jan 9, 2018

Member

Should this ever happen if manyArray is not null?

This comment has been minimized.

@igorT

igorT Jan 10, 2018

Member

lets add an assert

}
this._removeInternalModelFromManyArray(this._retainedManyArray, inverseInternalModel);
} else {
this.removeInternalModelFromOwn(inverseInternalModel);

This comment has been minimized.

@igorT

igorT Jan 9, 2018

Member

This seems to be common for both hasMany and belongsTo?

@@ -143,7 +167,26 @@ export default class ManyRelationship extends Relationship {
return;
}
super.removeInternalModelFromOwn(internalModel, idx);
let manyArray = this.manyArray;
// TODO: explain this madness

This comment has been minimized.

@igorT

igorT Jan 9, 2018

Member

❤️

return this.internalModel.modelName;
_inverseIsAsync() {
// TODO: clean this up; it's for implicit relationships
return this.isAsync;

This comment has been minimized.

@igorT

igorT Jan 9, 2018

Member

this seems incorrect?

let manyArray = this.manyArray;
// TODO: explain this madness
this._removeInternalModelFromManyArray(this.manyArray, internalModel, idx);
this._removeInternalModelFromManyArray(this._retainedManyArray, internalModel, idx);

This comment has been minimized.

@igorT

igorT Jan 10, 2018

Member

we should maybe do this in the async case?

This comment has been minimized.

@hjdivad

hjdivad Jan 11, 2018

Member

we discussed this OOB, but LMK if you still think there's an issue here

@fivetanley

This comment has been minimized.

Copy link
Member

fivetanley commented Jan 10, 2018

@hjdivad I tried this out in our app. We're still stuck in this scenario:

import Ember from 'ember';

export default Ember.Controller.extend({
  actions: {
    unload() {
      const clients = this.store.peekAll('client');

      // there is one record that matches this, so one record unloaded
      clients.filterBy('isCool').forEach((client) => {
        client.unloadRecord();
      });
      
      // here clients is [Model, Model, null]
      // null should not be in the array
      
      // error here because filterBy calls Ember.get on null to try to find hasDirtyAttributes.
      clients.filterBy('hasDirtyAttributes').forEach(client => {
        client.unloadRecord();
      });
    }
  }
});

It looks like some nulls are still getting stuck in the RecordArray created for this.store.peekAll('client').

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Jan 10, 2018

Thanks for the followup @fivetanley. I'll see about reproducing this in a test if you haven't already.

@hjdivad hjdivad force-pushed the hjdivad/restore-unloadRecord-client-side-delete-semantics branch from aa59926 to ad66b89 Jan 11, 2018

Wesley Workman and others added some commits Aug 17, 2017

Restore 2.12 semantics for sync unloadRecord
A number of use cases rely on unloadRecord's 2.12 behaviour of treating
unloadRecord as a client-side delete on the inverse side of a sync
relationship.

This pr restores that functionality, while retaining the
invalidate+refetch functionality for async relationships.

This behaviour is codified in a number of tests within
tests/integration/records/unload-test.js

Fix #5136, #5137

@hjdivad hjdivad force-pushed the hjdivad/restore-unloadRecord-client-side-delete-semantics branch from ad66b89 to 6e8a236 Jan 11, 2018

@hjdivad hjdivad requested a review from igorT Jan 11, 2018

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Jan 11, 2018

@igorT this should be good to merge; issues we discussed are handled.

@hjdivad

This comment has been minimized.

Copy link
Member

hjdivad commented Jan 11, 2018

@fivetanley this doesn't resolve the filter issue you mentioned in your previous comment; that will be addressed in a follow-on issue.

@igorT igorT merged commit b329a2c into master Jan 11, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@igorT igorT deleted the hjdivad/restore-unloadRecord-client-side-delete-semantics branch Jan 11, 2018

@sly7-7

This comment has been minimized.

Copy link
Contributor

sly7-7 commented Jan 12, 2018

@fivetanley I wonder if in your use case, calling toArray() after filter would change anything ?

@workmanw

This comment has been minimized.

Copy link
Contributor

workmanw commented Jan 17, 2018

This isn't tagged with the release channel. Can we please get this in 2.18 LTS?

@workmanw

This comment has been minimized.

Copy link
Contributor

workmanw commented Jan 28, 2018

Shameless bump @hjdivad @rwjblue -- Can we get this PR and #5249 into 2.18.1? I know DS doesn't have the same level LTS support as Ember, but it would be nice to have a place to jump to in the 2.x series.

@sly7-7

This comment has been minimized.

Copy link
Contributor

sly7-7 commented Feb 9, 2018

Or at least in 3.0 beta ?
BTW @workmanw did you try running your app against master ? On my side this seems to tackle all the regression I had after 2.12.2

@elgordino

This comment has been minimized.

Copy link

elgordino commented Feb 13, 2018

FYI This is included in 3.1.0-beta.1 https://github.com/emberjs/data/releases

@bmac

This comment has been minimized.

Copy link
Member

bmac commented Feb 13, 2018

I just released 2.18.1 and 3.0.1 with this pr. Thanks to @sly7-7 for pinging me about including this pr.

@workmanw

This comment has been minimized.

Copy link
Contributor

workmanw commented Feb 14, 2018

@bmac Thank you!!

@jlami

This comment has been minimized.

Copy link
Contributor

jlami commented Feb 15, 2018

@hjdivad What is the status of the followup on the problem @fivetanley was having?
I think this might be the same problem as I'm having. see also #5353 and #5354

Could somebody explain the reasoning behind the distinction in handling of the canonical state of sync and async relationships for unload? I'm inclined the client side should not be effected by this, as this is more of a server behaviour in my mind.

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