Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUGFIX beta] Fix unloadAll to also unload live records #5350

Closed
wants to merge 3 commits into from
Closed

[BUGFIX beta] Fix unloadAll to also unload live records #5350

wants to merge 3 commits into from

Conversation

dwickern
Copy link
Contributor

@dwickern dwickern commented Feb 6, 2018

unloadAll should also unload records which were fetched using findAll. Currently the unloaded records are destroyed but the record array isn't cleared. So calling findAll a second time after unloading will return the destroyed records and fresh ones (potentially duplicates of the destroyed records).

@dwickern
Fix unloadAll to also unload live records
1d9116c
@dwickern
fix watchModelTypes test
f99b70c 
there's now an additional "updated" event when the record is unloaded
during tear-down of the test suite
@dwickern
Merge branch 'master' into unload-live-records
320b830 
# Conflicts:
#	tests/integration/records/unload-test.js
joelalejandro
joelalejandro reviewed Mar 15, 2018
View reviewed changes
addon/-private/system/record-array-manager.js
@@ -76,6 +76,13 @@ export default class RecordArrayManager {
this.internalModelDidChange(internalModel);
}

recordWasUnloaded(internalModel) {
let array = this._liveRecordArrays[internalModel.modelName];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dwickern Since array isn't changing, maybe it's best to use const here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to match the existing code style

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. 👍

@sly7-7
Copy link
Contributor

sly7-7 commented Mar 16, 2018

LGTM, bu @hjdivad could you review this please ?
Also maybe prefix the commit with [BUGFIX beta] ?

@dwickern dwickern changed the title Fix unloadAll to also unload live records [BUGFIX beta] Fix unloadAll to also unload live records Mar 16, 2018
@runspired
Copy link
Contributor

I will look into this issue and review this PR as part of #5378. I suspect the underlying issue is a combination of one we have already fixed with the issue seen in peekAll.

@runspired runspired mentioned this pull request Mar 17, 2018
Merged
@dwickern
Copy link
Contributor Author

@runspired the test in this PR fails against your branch

@runspired
Copy link
Contributor

I dug in and while this fix appears to work it does not address an underlying issue:

store.findAll appears to put records into a fundamentally different state than store.push. I'm looking into why this is, as they should be in the same state.

@runspired
Copy link
Contributor

The issue appears to be that records need to have been accessed to be unloaded correctly. findAll puts records into the record-array, but does not materialize them. Using store.push has the side-effect of materializing them. I was able to build simpler test reproductions once I narrowed in on this.

runspired added a commit to runspired/data that referenced this pull request Mar 17, 2018
@runspired
add test and fix for emberjs#5350
55fd924
@runspired
Copy link
Contributor

I found the underlying issue and resolved it here: 55fd924

Thanks for the failing test and fix attempt, it was extremely helpful in narrowing in on the root cause.

@runspired runspired closed this Mar 17, 2018
This was referenced Mar 17, 2018
Closed
Closed
bmac pushed a commit that referenced this pull request Mar 26, 2018
@runspired @bmac
[BUGFIX] resolve issues with RecordArray sync for peekAll (#5378)
fc327b2 
* [BUGFIX] resolve issues with RecordArray sync for peekAll

* remove unneeded part of fix

* Adds a test for #5167

* add test and fix for #5350

* port test from #5095

* fix filter test for older versions of Ember
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelalejandro joelalejandro joelalejandro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dwickern @sly7-7 @runspired @joelalejandro