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.

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

[TEST] - failing test to push records with async hasMany rel. after they have been destroyed on client-side #6600

Conversation

eodb
Copy link

@eodb eodb commented Oct 16, 2019

This is a failing qunit test (named ''Records with an async hasMany can be pushed again after they were destroyed on client side'), to illustrate that the async hasMany relationships of a record are not as one would expect when the record(s) are pushed a 2nd time from the server (we are using web sockets) after they were previously destroyed on the client side.

In our application, records tend to too large so we have to destroy the child records when leaving some views in order to reduce the memory footprint of the browser. Next time when entering the same view, the records are pushed again from the server. Other records are long-lived and are reused in other views.

Starting from ember-data v3.13.1, we noticed that whenever the destroyed records where pushed again, one of the async hasMany collections is ever growing. The ids of the records within the collection are identical and accumulating on every push.

This qunit test within the 'delete-record-test' can be used to easily reproduce/test the issue:
1 record ('Company') is a long lived record, the other two records ('Group' and 'Employee') refer to one-other using async hasMany relations. Group and Employee are destroyed and pushed once more. The Group ends up with 2 (identical) employees instead of 1.

dependabot-preview bot and others added 29 commits November 26, 2019 06:27
* [CHORE] Move _modelForMixin to Model package

* ensure modelForMixin is available
…emberjs#6797)

* belongsTo tests for links

* complete belongsTo tests

* first set of hasMany tests

* complete hasMany tests

* fix lint

* bugfix

* Be more positive
* [BUGFIX] cleanup rollup warnings

* fix lint

* make throw
* improve packaging

* add log to figure out CI and update ilios

* make extra sure we aren't trolling ourselves

* cleanup

* improve read-file
I saw in the [documentation](https://api.emberjs.com/ember-data/release/modules/@ember-data%2Fserializer) the example code for the `serialize` method and it shows the `snapshot.id` property as a function instead of a string. I [double checked it](https://github.com/emberjs/data/blob/master/packages/store/addon/-private/system/snapshot.js#L65) to be sure it is just a string :) I hope this is the right place to make the fix to the documentation.
- changes the comment-bot to update existing comments instead of deleting old comments
- fixes the bug that was preventing deletion of old comments
- fixes the performance analysis script to also post a comment correctly (it's been failing to do so although trying for some time)

You can see the effects of the updates (edit history on the pr comments) within this PR. Overall it seems like a good change, although I dislike how the comment time stamp remains the time of the very original comment instead of showing the time edited. This makes it harder to judge quickly if it is the most recent analysis, although checking the commit sha that is printed will verify.
…erjs#6823)

* [FIX] partner-tests, use lock-file but keep yarn change and no-cache change

* fix comment script

* wat

* but seriously

* sdf

* sdf

* side effects are what you bash your head over

* dont use lock file for ember-resource-metadata
…emberjs#6824)

* [TEST] generateIdForRecord tests for adapter encapsulation side quest

* lint fix

* address comments
* 11

* compression deltas

* cleanup

* it helps to return the 11
…l package (emberjs#6822)

* [CHORE] Move PromiseBelongsTo, PromiseManyArray and ManyArray to model package

* fixes

* reduce inter-dependency by moving proxyToContent

* cleanup
* build both IE11 and modern

* cleanup

* fix

* fix path

* more cleanup

* fix comment formatting

* add no-rollup check too

* fix display

* sweet lines

* 🤦
* Fixes and cleanup for actions

* fix node tests

* maybe

* shrink assets to upload

* why?

* fix output path

* allow max-transpilation to run a little longer

* dont error out for allowed failures

* set timeout specifically for tests so artifacts will upload

* upload

* cache less

* fix
* add ie11

* ensure we are always using RSVP Promise

* tweaks

* ie11 cleanup

* freezit

* just in case

* fix lint

* always run production

* fix

* dont run TODO tests in Prod

* allow iteration

* fix

* fix startsWith

* almost there

* turn on waiter for more things and fix assert

* fix?

* maybe fix?

* fastboot test helpers dont work in production so we cant test fastboot+ie11
Bumps [eslint](https://github.com/eslint/eslint) from 6.7.1 to 6.7.2.
- [Release notes](https://github.com/eslint/eslint/releases)
- [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md)
- [Commits](eslint/eslint@v6.7.1...v6.7.2)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* [CHORE] Use model name instead of store in isPrimaryType (emberjs#6338)

* Use model name instead of store in isPrimaryType

* Fix property naming error

* fix tests

* deprecate old private method rather than deleting
…erjs#6831)

resolves emberjs#6431

Bugfix: jQuery+fastboot

The added tests for fastboot revealed that fastboot+jQuery was broken because our computed property for getting the fastboot service wasn't setting and updating it's value properly leading to it always being false. This led us down the wrong code-path and ultimately meant that the url we provided to najax wasn't parsed correctly and would error.
dependabot-preview bot and others added 17 commits April 6, 2020 10:53
Bumps [acorn](https://github.com/acornjs/acorn) from 5.7.3 to 5.7.4. **This update includes a security fix.**
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@5.7.3...5.7.4)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [babel-eslint](https://github.com/babel/babel-eslint) from 10.0.3 to 10.1.0.
- [Release notes](https://github.com/babel/babel-eslint/releases)
- [Commits](babel/babel-eslint@v10.0.3...v10.1.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [ember-cli-htmlbars](https://github.com/ember-cli/ember-cli-htmlbars) from 4.2.2 to 4.2.3.
- [Release notes](https://github.com/ember-cli/ember-cli-htmlbars/releases)
- [Changelog](https://github.com/ember-cli/ember-cli-htmlbars/blob/master/CHANGELOG.md)
- [Commits](ember-cli/ember-cli-htmlbars@v4.2.2...v4.2.3)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 3.7.5 to 3.8.3.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](microsoft/TypeScript@v3.7.5...v3.8.3)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
…ttps://github.com/eodb/data into eddy/push-async-hasMany-after-client-side-deletion

Fix eslint warnings
This reverts commit 602b3ed.
@eodb
Copy link
Author

eodb commented Apr 9, 2020

@runspired Thanks for the integration of #7109. This also fixes this unit test. I suggest to integrate it as a test to restore the async relationships after some child records of a long-living record have been deleted and are restored on a next push. A common scenario in our products to free up memory. I rebased the pull request with master, so I hope Integration is straight forward.

@runspired
Copy link
Contributor

closing in favor of #7258 or a follow up after #7475

@runspired runspired closed this Apr 23, 2021
snewcomer added a commit that referenced this pull request Apr 24, 2021
snewcomer added a commit that referenced this pull request Apr 24, 2021
snewcomer added a commit that referenced this pull request Apr 24, 2021
runspired pushed a commit that referenced this pull request Apr 29, 2021
snewcomer added a commit that referenced this pull request May 1, 2021
snewcomer added a commit that referenced this pull request May 5, 2021
runspired pushed a commit that referenced this pull request May 7, 2021
runspired pushed a commit that referenced this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.