unloadRecord is not under as much test coverage as we think it is

[runspired]

I recently dove into a few scenarios where unloadRecord was behaving badly but which seemed as though they should have failed tests. I discovered:

• our tests do not cover deleteRecord and destroyRecord scenarios
  • as a subset of this, it turns out that destroyRecord + unloadRecord moves the record to an empty non-deleted, non-destroyed (simply non-loaded) state 😱
• our tests do not cover createRecord scenarios where relationships are involved
• our unloadRecord tests generally do not test that in the scenarios were we expect an unload to fully purge the record and internalModel (e.g. disentangled + unloaded) that this actually happens, only that the appearance of it happens.

[runspired]

@fivetanley I suspect this is why our destroyRecord also unloading PR produced failures for you in #5455

Also related: #5006

[Gabbyjose]

Hello, I was wondering if this issue is still open? I'm a beginning and would be interested in learning more!

[Vincz]

Hi! I tried the latest ember-data version (3.2.0-beta.2) and the problem described here #5328 is still here. I'm stuck in 2.12 because of this.
The scenario is as follow:

1. Receive a bunch of model to delete from a websocket
2. Peek the record with store.peekRecord
3. Unload the record with object.unloadRecord()

After that, I get a bunch of 404, ember data is trying to get the record again.
In 2.12 everything is ok.

[runspired]

@Gabbyjose we're still working on this one, I've added some tests but we're not quite done. Sorry for not getting back to you sooner, last month was a bit... out there. If you'd still like to help I'd love to pair with you!

[runspired]

@Vincz this issue has nothing to do with #5328. unloadRecord isn't for "forgetting" records, it is for marking them as stale (e.g. needs to be reloaded). However, there is a way to delete records which we will codify soon (been working on an RFC). The gist of it is here

[Gabbyjose]

@runspired no worries at all - crazy what happened! i'm definitely still happy to take a stab at this and work with you. please let me know what works best for you!

[runspired]

@Gabbyjose awesome to hear! Is there a good time tomorrow to sync for 30 minutes or so? I'm on Pacific Time. You can find me on Slack/Discord/Twitter/Gmail as @runspired if that's easier to chat to set something up.

[Vincz]

@runspired Thank you for sharing your code. I tried it and it seems to work just fine :) I'm just worried about relationship without inverse (inverse: null). Will it work ?

[amk221]

I think I have one of the scenarios that is not covered. I am trying to upgrade from 2.18, in which I have good, passing test coverage. I've ruled out a couple of issues (#5517, ember-fastboot/ember-cli-fastboot#223) But have eventually ended up here.

[runspired]

@amk221 if you can describe it, that would be awesome! There are a couple of known outstanding issues / areas of coverage needed, but most of the ones above have been added.

[amk221]

I think there are a couple of things, all demo'd in the above repo.

1. in 2.18 when I call rels.invoke('unloadRecord'), a second call to myModel.rels does not make a request for data. But after 2.18 it does.
2. Unloading records from a hasMany that has had records created in it.

[runspired]

The following PRs were created or include tests that address this:

• [BUGFIX] destroyRecord should also unload #5455
• Add tests for destroyRecord in hasMany/belongsTo relationships #5480
• Add tests for createRecord+unloadRecord #5528
• [BUGFIX] fix and tests for belongs-to proxy not properly updating #5533
• Acceptance Tests for belongsTo #5566
• Add failing test when changing parent and unloading original parent #5571
• Failing test for unloadAll scenario with hasMany relationship that ends up wrong #5572

#5400 should be investigated for additional coverage.

Closing as I think our coverage gap is no longer so severe.