Data: model.unloadRecord does not change state to => root.deleted.saved ( as it used to )

[danielspaniel]

Maybe you already know this but
-> model.unloadRecord()
does not set the model into deleted state like it used to.
After unloadRecord the model is still in 'root.loaded.saved' state.

[stefanpenner]

unloadRecord moving stuff into root.deleted.saved seems like it was a bug. unloadRecord is more like... "please if possible forget this record"

If you want to delete something, please use deleteRecord(), if you want to delete without hitting the server. You could fake it in the adapter method.

I suspect we should likely explore "client side delete" as dedicated method, but that likely will require an RFC.

Hopefully ^ helps a tad. Sorry for the delay.

---

I am closing this now, if I misunderstood please let me know, and I can reopen.

[sly7-7]

It just sounds "fun" that the term unloadRecord leaves the model in the loaded state.

[danielspaniel]

Good reasoning Stephan about unload just removes and does not delete.
I was confused because of the counter intuitive name of 'unloadRecord' which, as @sly7-7 noted, adds a bit of wild and carefree to ember data api.

[sly7-7]

@danielspaniel It seems like in https://github.com/emberjs/data/pull/5092, @stefanpenner adressed the client side delete semantic :)

[danielspaniel]

@sly7-7 => good reference .. I got even more reasoning now :)

[stefanpenner]

I think the previous state of unloadRecord had some quirks that make it tricky to reason about its intentions. Hopefully what I said in #5092 sounds reasonable, if not let me know :)

[workmanw]

@stefanpenner et al. I would ask that you consider reopening this. This turned out to be a sizable issue for us for a whole variety of reasons. The biggest and hardest to work around is handling realtime events.

In our application, we have a realtime service that is constantly listening for CRUD type events on various realtime channels. If it sees a model that is in DS Store's cache is deleted, it will unload the record. Many of our application utilities that do querying w/ DS, have observers or computed properties to filter out models which are deleted. This makes the DELETE part of realtime possible.

The problem with this change is I now have no way to tell, given a model instance in a cached list, if it was unloaded (realtime deleted). Thus I have no way to filter them out.

[sly7-7]

@workmanw I don't have tested yet, but it seemed to me the work in #5092 put the unloaded records in the deleted state again.

[workmanw]

I tested using v2.14.9 which does have this change (v2.14.9) and it still does not work. Calling model.unloadRecord does unload it, but leaves it in the state: root.loaded.saved. Maybe there is something I'm missing here.

Really the need I have is just to know if a model is unloaded so I can filter it out. It doesn't have to be in the deleted state, but something that can be used to uniquely identify it.

Until this gets resolved I'm still stuck on 2.12 with no end in sight. It's been a tough few releases with ember-data.

[stefanpenner]

Until this gets resolved I'm still stuck on 2.12 with no end in sight. It's been a tough few releases with ember-data.

@workmanw That sucks and I would like to figure out how to make this not suck for you. Let me share my thoughts, and we can chat/figure out whats appropriate re: step forward. (Again thank you for your patience recently, I hope my recent work gets ED back on track re: stability).

Currently, I am not of the belief unloadRecord should put records into a deleted state, as the design is more "please forget you fetched me" not "please pretend you were deleted", as it is quite conceivable to re-discover an unloaded record at some future time. I believe conflating "please forget me" and "please delete me" would be unfortunate.

Question: Would a method that actually does a clientSide destroy be sufficient to mitigate your pain? Does that method feel ok, or does it feel hacky?

[stefanpenner]

@workmanw the way I could currently implement this (client only delete) right now (with only public API) would be something like:

// realtime channel handler
function updateFromChannel(store, update) {
   const { id, type, operation) = update;

  if (operation === 'DELETE') {
    let record = store.peekRecord(type, id);
    if (record) {
      record.deleteRecord({ adapterOptions: { clientOnly: true });
    }
  }

  // handle the other updates
}

// adapter

deleteRecord(store, type, snapshot) {
  const { adapterOptions } = snapshot;
  if (adapterOptions && adapterOptions.clientOnly) { return {}; }
  return this._super(...arguments);
}

But I could imagine this being made nicer/first-class.

[workmanw]

@stefanpenner and I talked about this on slack. I'm going to use the workaround he suggested. For reference, one other alternative solution that came out of our discussion would be to add a root.unloaded state for models (and isUnloaded). So it could be both not deleted and not root.loaded.

EDIT: To be clear, we're not pursuing the root.unloaded at the moment. But it's an idea for the backburner if this issue resurfaces in some other way.

[runspired]

This should result it in being in the root.empty state not root.loaded.saved, something seems wrong. @workmanw

[workmanw]

@runspired Here is a twiddle showing that: https://ember-twiddle.com/7cfe7142524edb3a03755fb913dc035f?openFiles=controllers.application.js%2C

Notice that after I unload the record, it stays in the last state it was in. In this gif, after unloading, I try to re-dirty the record to demonstrate it is unloaded.

[runspired]

@workmanw correct, what I'm saying is that root.loaded.saved and root.deleted.saved are both wrong. An unloaded record should be in root.empty, and in fact the code sets it be so, meaning that likely something else is setting it back or causing the change to not be flushed. See https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L365 which is called here: https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L377

Hrm, actually looking at that, if _record does not exist this would prevent it from entering the correct state. I will try to reason through this this week.

[workmanw]

I know this is out of scope, but even root.empty feels wrong to me. I'd really like to see a root.unloaded introduced so that I can easily filter out unloaded and deleted records. I could, and will, also filter out root.empty, it just feels ambiguous.

Unless the only way for a record to be in root.empty is if it's unloaded, if that's the case I rescind my prior statements.

[workmanw]

@runspired Also, should we reopen this issue?

[runspired]

@workmanw I believe we should reopen this as it seems to be the root cause of a number of issues.

Fwiw we filter out empty for you in most cases. In cases we don't, accessing it would cause it to be fetched.

Root.empty quite literally just means this internal model is a reference to a resource that has not been loaded (or in this case has been unloaded), and needing to retain the reference is why we didn't want completely eliminate it, as it would just be instantly recreated by any relationships.

If we transitioned to root.empty I believe this would solve most of the problems we've seen but introduce a new one (possibly but not necessarily solvable with root.unloaded) in that if a relationship did need the record it would immediately fetch it. Personally, I think this immediate fetch is likely to be the more desired behavior.

[runspired]

I'm curious what @hjdivad @spenner and @igort think.

[workmanw]

Fwiw we filter out empty for you in most cases. In cases we don't, accessing it would cause it to be fetched.

Yeap. The only use case I have is maintaining my own list of models for infinite paging. Because the ember-data record arrays don't allow you to manually add to them, I have my own utility array.

[hjdivad]

@runspired @workmanw the current state of the internal model in your twiddle is root.empty. You're not seeing it update in the twiddle because it's not set on the record during dematerialization (as it is in transitionTo).

If you're filtering on destroying/destroyed records you would filter the record out in this case.

[workmanw]

@hjdivad That makes sense and I can get onboard with that.

More than anything, I think I want these behaviors to be cemented and not part of the "intimate API" that is subject to change.

[hjdivad]

@workmanw yes absolutely. We discussed this on our call this week; from what I can tell the difficulty stems from people using unloadRecord for pretty disparate use cases, but all of which are totally reasonable (eg indicate safe to free, absolutely free even with other references, client-side delete &c.).

Even if we're more explicit about the semantics of unloadRecord (or any api for that matter) we need to make sure the other use cases people want have a reasonable path forward.

[artavdalyan]

Ember Data version 3.7.0
When I am use the below implementation, when I have model length == 1 ,
I have done model = store.peekRecord(modelName, id), model.destroyRecord(), after that
store.peekall(modelName) length == 0 , but store.peekRecord(modelName, id) returns deleted object

deleteRecord(store, type, snapshot) {
const { adapterOptions } = snapshot;
if (adapterOptions && adapterOptions.clientOnly) { return {}; }
return this._super(...arguments);
}

function updateFromChannel(store, update) {
const { id, type, operation) = update;

if (operation === 'DELETE') {
let record = store.peekRecord(type, id);
if (record) {
record.deleteRecord({ adapterOptions: { clientOnly: true });
}
}