Fix client-side delete + resurrection

[hjdivad]

Previously relationships were only cleared for new records. Now they
are cleared for destroyed records as well, allowing destroyed records to
be pushed back into the store and properly update their inverse
relationships.

[hjdivad]

#5136 / #5137 are expecting unloadRecord to client-side delete (ie remove themselves from non-unloaded inverse relationships).

This use case is supported per documentation via a client-side delete, ie

• delete the record
• have the adapter not send a request

However, some client-side delete use cases require resurrection (ie client-side deleting the record and then pushing the same record back into the store and repopulating inverses).

This behaviour was changed in #5058, which aimed to keep local state in relationships after record deletion

[hjdivad]

cc @runspired @workmanw

[hjdivad]

@workmanw i believe I've captured your use case in public api in the test, but have also verified in your repo https://github.com/workmanw/unload-relationships-bug

If you can confirm this solves your issue that would be great but in either case thanks for your diligence and patience.

[stefanpenner]

Reviewed \w @hjdivad, this LGTM.

[stefanpenner]

Glad we can drop this extra complexity.

[runspired]

I suspect I didn't add an adequate enough test for why that extra guard was needed. I'll dig in this morning.

EDIT: (FTR I too am hoping we can drop the guard, I'm just suspicious because at the time I added it things broke without it).

[runspired]

After reviewing the tests +1 LGTM

[hjdivad]

@runspired the reason this doesn't regress the local-changes preservation is because it's captured by flushCanonical

[runspired]

@hjdivad records in the isNew state are covered, but not records not in that state. I do believe I have a test though that is passing anyway.

TL;DR what I'm saying is:

• a hasMany B [b1]
• push existing B (b2, not isNew) onto a
• now a hasMany B [b1, b2] with canonical state [b1] and current state [b1, b2]
• delete b1
• flushCanonical
• bug would be if the resulting state is a hasMany B [] instead of a hasMany B [b2]

[workmanw]

So using this PR with the suggested changes for "client side delete" and "unloading" does work around the issues of #5136.

comment.destroyRecord({ adapterOptions: { clientSideDelete: true } }).then(() => {
  comment.unloadRecord();
});

In my application, we have a base model, so I'd probably just create a utility function for this behavior. E.g. comment.clientSideDelete(). But that got me thinking, if this is the official work around to what was previous functionality, why not add this behavior in ember-data. It seems like that would ease the transition for many people who are currently stuck. You could add it in ember-data without having to touch the adapter code. In other words, clientSideDelete would by pass the adapter's deleteRecord call altogether. Simplifying the operation and not requiring people to modify their adapters.

[hoIIer]

this may possibly fix the issue we've been having which I submitted here... #5237

[hjdivad]

@workmanw we're going to restore the previous semantics of unloadRecord for sync relationships.

Your suggestion of having some kind of "skip adapter" functionality is a good one; as it happens @stefanpenner was discussing the possibility of doing this as well for something that's common and currently not super ergonomic with the public API.

[hoIIer]

hey so I just tested this thinking it might address the issue of "natural key" which I submitted in #5237 but it didn't solve the problem...

this is the assertion that is triggered when I try to create a record using a natural id which was previously used for a now destroyed record...

it seems like this is related but I did a quick run through and I still get the assertion. any feedback appreciated

[hoIIer]

@hjdivad any movement on this or thoughts around my previous comment?

[hjdivad]

@erichonkanen The only thing I'm going to do before merging this pr is trace the delete with local additions case to confirm this is tested as expected.

I believe your issue #5237 has the same root cause as #5136 which is the next thing I'm aiming to address.

[workmanw]

I was hoping to see this make it's way into 2.17, but admittedly that might have been a little ambitious. Can we set a goal of having this resolved in 2.18?

[hjdivad]

Walked through this with @rwjblue and we've confirmed the regression that flag was meant to capture is indeed still tested and works.

Additionally, the case that @runspired outlined also still works, but only because deletes won't trigger flushCanonical.

It remains the case that ManyArray.flushCanonical discards local changes for non-new records, but that's not the issue that the isNew flag was meant to address.

[hjdivad]

Failing test for the above issue: #5269

[workmanw]

@hjdivad et al, thanks a lot for all your hard work on this. It really is appreciated.

Hopefully I'm not pestering too much, but I'm curious what the next steps are toward resolving: #5136 . We had talked on slack and on workmanw/unload-relationships-bug#1 about how this was the critical step to resolving these issues. I believe the net out of that convo was once this issue was resolved, then it could be worked around by introduced adding custom adapter options on the application side.

Example:

// adapters/application.js
deleteRecord(store, type, snapshot) {
  let options = snapshot.adapterOptions || {};
  if (options.clientSideDelete) {
    return resolve();
  }
  return this._super(store, type, snapshot);
}

// controllers/comment.js
actions: {
  unloadComment() {
    let comment = this.get('comment')
    if (comment) {
      comment.destroyRecord({ adapterOptions: { clientSideDelete: true } }).
        then(() => comment.unloadRecord());
    }
  }
}

I'm curious what the general thoughts are on the state of this issue. Will application owners need to make these changes to restore functionality, or will there be further improvement to make this easier?

Thanks again!

[hoIIer]

Not to hijack but the issue above regarding loading/unloading is (I think) what is preventing us from moving past 2.12.x and is something we are really hoping gets resolved... our functionality works great on 2.12.x but there are other issues and obv we want to be on the latest

[hjdivad]

I'm curious what the general thoughts are on the state of this issue. Will application owners need to make these changes to restore functionality, or will there be further improvement to make this easier?

@workmanw no, we're going to restore the client-side delete semantics of unloadRecord for sync inverse relationships.

@rwjblue and I had some WIP on this Friday.

[workmanw]

@hjdivad @rwjblue Awesome. Thanks for the update and all your hard work on this stuff!