failing test for #5343

[jlami]

Don't know if this is exactly a test for the first scenario in #5343, but it does fit my scenario that throws the same error.

It fails on an observer on a hasMany that has one member unloaded, when that observer immediately reads the computed property again. This tries to refetch the now dematerializing record.

[sly7-7]

Thank you for the scenarii you describe here and in #5353 . Here I think the runloop make it work, because it "delays" the observer execution (in emberjs, observers, unlike cps are sync).
That beeing said, I don't know what could be done in ED to fix those both "corner case". cc/ @hjdivad

[jlami]

The problem with trying to solve this with the runloop is that this currently happens when using normal computed properties that use the hasMany. So trying to fix this would involve many other components.

More and more I am thinking that observers should have a special place in the runloop, so computed properties will never be triggered before ember (and ember data) is ready for it. This might also solve some double render bugs I'm seeing.

[sly7-7]

I don't even know what I'm talking about, but maybe you could take a look at this new ember-concurrency's PR: machty/ember-concurrency#205, so maybe you can leverage it to wait for the 'observer' to fire ?

[jlami]

That does look cool, but I think that might be more interesting for scenarios that interact with observers directly.

The problem I'm having is that a normal Ember.computed.mapBy('hasManyRelation', 'someProperty') is causing troubles because it uses observers under the hood.

[sly7-7]

So what you describe here, is that there is some sort of race condition when using both Ember.computed.xxx functions and async relationships ? In that case, maybe ember and/or ember-data could leverage ember-concurrency principles to handle those case ? I'm sorry those are just ideas, perhaps it could be simpler. But I think those problem are tricky, otherwise there would already have been resolved. If you have ideas of implementation/fixes in ember-data at least, they are obviously welcome :)

[jlami]

I think it comes down to how ember-data currently handles the dematerializing state. An internal model is flagged as such, but can still be in the ManyArray if the relation is async. The unloading of the record does trigger the hasMany computed property observer, which somehow triggers it to be called at a point in time where ember-data is not ready to refetch the record. This results in the null reference.

I had partial success in fixing the bug by preventing the fetch of dematerializing records in the ManyArray, but this still left an empty record in there. But that might be 'correct' behaviour now that the ManyArray is kept for potential reloading.

[sly7-7]

I think this would be awesome if you could work on that either with @hjdivad or @runspired

[jlami]

It seems like the solution offered in #5328 will prevent this bug from surfacing. But I'm not really keen on using _internalModel in an addon.

Maybe the deletion really updates the canonical members, so the old record never gets used at recompute time. And if only unloading the old (still dematerializing) record is kept in the array, which will result in incorrect behaviour when it is trying to unload if the unload is not really done.

I guess this is the change in 6e8a236 but I'm still not sure the new async behaviour of the hasMany is needed. But even if it is, the unloading should work.

Maybe https://github.com/emberjs/data/blob/master/addon/-private/system/model/internal-model.js#L338 should not test for _isDematerializing and always give a new record? Or the unloading should be cancelled at that point?

[sly7-7]

@jlami For me using _internalModel in ember data internals is ok. So if you think you can add this line in the first place of unloadRecord, and it passes all the tests, then it might be a good PR :)

[jlami]

Yes, in ember-data it would be ok, but the fix would be for outside ember-data. The calling code that wants to notify the store of a deleted record will have to be altered. Where it only unloaded the model before (which still works for sync relationships), it has to mark the record as deleted with the _internalModel before it can be unloaded for this bug to be circumvented. That does not sound like a good idea to me.

[sly7-7]

Well, what I mean actually, is to put this line here: https://github.com/emberjs/data/blob/master/addon/-private/system/model/states.js#L578
internalModel.transitionTo('deleted.saved')
What do you think about that ?

[jlami]

That might fix the error, I'll have to test that. But it does go against the new way of thinking that a async ManyArray should keep items to be able to rematerialize. So this would definitely break some existing test.

[sly7-7]

Well, that was my two cent. To be honest, now, I just feel ignorant about what to do next. You seem to have tons of idea, and also how to implement them. Please, just do it if you can, send a PR, and I think that either @hjdivad or @runspired or @stefanpenner will kindly review it :)

Maybe you could do one pr for all the bugs you have, if possible. And linking to those issues.

[jlami]

Well, I'm more spitballing here. It is good to have some feedback. So thanks.
I'm just afraid I don't know enough of the internals to really change handling of a flag like _isDematerializing.

[sly7-7]

I would just say "Do as you think it would be good enough" :)

[hjdivad]

@jlami thank you very much for the detailed test! This really helps with investigating these issues.

I've opened a couple of prs (#5376 #5377) to handle #5343.

#5377 is the one that resolves the issue highlighted here.