Fix unload record #2870
Fix unload record #2870
Conversation
drogus
force-pushed
the
fix-unload-record
branch
2 times, most recently
from
2dd9918
to
ff85a35
Compare
There are a few inconsistencies in how unload record works:
* it differs from unloadAll, because it doesn't destroy records
* it sets a record in deleted.saved state, which may be wrong if you
want to unload record, which is not actually deleted
* it doesn't dematerialize records, so they're not garbage collected and
stay in the `idToRecord` array forever (which may also cause
problems if you want to load a record with the same id later)
This commit fixes these issues by calling _dematerializeRecord and
destroy when unloading a record.
drogus
force-pushed
the
fix-unload-record
branch
from
ff85a35
to
53834a7
Compare
|
I should probably add that currently the tests fail, because we don't remove records from record arrays when a record is destroyed or when it's destroying. It will pass once #2867 is fixed. |
|
@igorT is this thing still needed? I haven't followed ED development lately, so I'm not sure how it looks like in context of other changes. |
|
@drogus I would say that yes it is still needed. But now there a record and internal-record to deal with. @krisselden was asking about exactly this today in slack :) |
|
@drogus do you have the bandwidth to pick this back up? |
|
@fivetanley yup, although I would need to get familiar with ED internals again. I can look at it over the weekend. Do you know if the algorithm that @igorT outlined here is still up to date? |
|
@drogus Do you know needs to happen to get this pr into a place where it can be merged? |
|
I'm going to close this issue since there hasn't been any response in a while. Feel free to reopen if there is an update. |
This targets #2859 and #2862. When I started my work on this I only wanted to fix #2859, but both issues are connected and I believe that it's better to fix them at once. You can't easily change transitioning to
deleted.savedwithout doing something else that removes record from record arrays, which can be achieved by destroying a record. Additionally, destroying a record on unloading without dematerializing it doesn't really make sense, so I also added that.One more thing that should be a motivation for this change is that
dematerializeRecordand it mentions usingunloadRecord, which isn't an equivalent at the moment.