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] Fix flushing of pending saves, that include a deleted record #4994

Merged
merged 1 commit into from Jun 18, 2017
Merged

[BUGFIX] Fix flushing of pending saves, that include a deleted record #4994

merged 1 commit into from Jun 18, 2017

Conversation

simonihmig
Copy link
Contributor

Fixes #4993

Seems the regression was introduced with #4668, by refactoring from a forEach loop to a for loop, where we cannot simply return anymore: 063b3e5#diff-fd4c34d9a34dfde73cd3413128a3c973R1866

@simonihmig
Copy link
Contributor Author

No idea why AppVeyor has failed, I cannot see any errors in their log...

@simonihmig simonihmig changed the title Fix flushing of pending saves, that include a deleted record [BUGFIX] Fix flushing of pending saves, that include a deleted record May 27, 2017
@simonihmig
Copy link
Contributor Author

Unfortunately the latest releases do not include this. As the problem is quite obvious and the fix uncontroversial, I hope this can be merged sooner than later, so pinging a few folks here... @bmac @stefanpenner @igorT @runspired @pangratz

@runspired
Copy link
Contributor

👌

@bmac bmac merged commit 93e4d65 into emberjs:master Jun 18, 2017
bmac pushed a commit that referenced this pull request Jun 19, 2017
@simonihmig @bmac
[BUGFIX beta] Fix flushing of pending saves, that include a deleted r…
2d03eca 
…ecord (#4994)

Fixes #4993
(cherry picked from commit 93e4d65)
@bmac
Copy link
Member

bmac commented Jun 19, 2017 •
edited

Sorry about that @simonihmig. I've been busy and missed this pr. Ember Data 2.14.1 has been published with this fix.

@simonihmig simonihmig deleted the fix-flush-pending-save branch June 19, 2017 15:55
@simonihmig
Copy link
Contributor Author

@bmac no problem, thanks for the quick follow up release! 👍

sudowork pushed a commit to sudowork/data that referenced this pull request Jun 20, 2017
@simonihmig @sudowork
[BUGFIX beta] Fix flushing of pending saves, that include a deleted r…
5cc9d2d 
…ecord (emberjs#4994)

Fixes emberjs#4993
@stefanpenner
Copy link
Member

this broke the build

@stefanpenner
Copy link
Member

integration/deletedRecord - Deleting Records: Will resolve destroy and save in same loop (

Died on test #2     at Module.callback (http://localhost:4200/assets/tests.js:10865:19)
    at Module.exports (http://localhost:4200/assets/vendor.js:123:32)
    at requireModule (http://localhost:4200/assets/vendor.js:38:18)
    at TestLoader.require (http://localhost:4200/assets/test-support.js:4675:7)
    at TestLoader.loadModules (http://localhost:4200/assets/test-support.js:4667:14)
    at Function.TestLoader.load (http://localhost:4200/assets/test-support.js:4697:22)
    at http://localhost:4200/assets/test-support.js:4580:18: Assertion Failed: normalizeResponse must return a valid JSON API document:
	* One or more of the following keys must be present: "data", "errors", "meta".@ 24 ms
Source: 	
    at assert (http://localhost:4200/assets/vendor.js:16279:13)
    at Object.assert (http://localhost:4200/assets/vendor.js:27947:34)
    at normalizeResponseHelper (http://localhost:4200/assets/vendor.js:84257:20)
    at http://localhost:4200/assets/vendor.js:88804:19
    at Backburner.run (http://localhost:4200/assets/vendor.js:11296:23)
    at Backburner.join (http://localhost:4200/assets/vendor.js:11322:23)
    at http://localhost:4200/assets/vendor.js:88800:23
    at tryCatch (http://localhost:4200/assets/vendor.js:69108:14)
    at invokeCallback (http://localhost:4200/assets/vendor.js:69123:15)
    at publish (http://localhost:4200/assets/vendor.js:69091:9)
afterEach failed on Will resolve destroy and save in same loop: Assertion Failed: You can only unload a record which is not inFlight. `<person:null>`@ 25 ms
Source: 	
    at assert (http://localhost:4200/assets/vendor.js:16279:13)
    at Object.assert (http://localhost:4200/assets/vendor.js:27947:34)
    at Object.assertAgainstUnloadRecord [as unloadRecord] (http://localhost:4200/assets/vendor.js:77332:18)
    at InternalModel.send (http://localhost:4200/assets/vendor.js:83296:30)
    at InternalModel.unloadRecord (http://localhost:4200/assets/vendor.js:83063:10)
    at InternalModelMap.clear (http://localhost:4200/assets/vendor.js:84067:15)
    at IdentityMap.clear (http://localhost:4200/assets/vendor.js:84175:16)
    at Class.unloadAll (http://localhost:4200/assets/vendor.js:87780:25)
    at Class.willDestroy (http://localhost:4200/assets/vendor.js:88701:10)
    at Class.superWrapper [as willDestroy] (http://localhost:4200/assets/vendor.js:51006:22)
Expected 1 assertions, but 3 were run@ 26 ms
Source: 	
    at Module.callback (http://localhost:4200/assets/tests.js:10865:19)
    at Module.exports (http://localhost:4200/assets/vendor.js:123:32)
    at requireModule (http://localhost:4200/assets/vendor.js:38:18)
    at TestLoader.require (http://localhost:4200/assets/test-support.js:4675:7)
    at TestLoader.loadModules (http://localhost:4200/assets/test-support.js:4667:14)
    at Function.TestLoader.load (http://localhost:4200/assets/test-support.js:4697:22)
    at http://localhost:4200/assets/test-support.js:4580:18

@stefanpenner
Copy link
Member

fixed with -> #5030 i think

@bmac
Copy link
Member

bmac commented Jun 22, 2017

Sorry about that @stefanpenner. We should look into setting up @homu on this repo again, because the pr looked green when I merged it.

@stefanpenner
Copy link
Member

@bmac ya github is misleading

@simonihmig
Copy link
Contributor Author

@bmac @stefanpenner Sorry about that! 😩

Still not sure why tests were passing locally and on Travis when PR was created? Or did something on master change meanwhile that caused this?

surfacedamage pushed a commit to Aperta-project/Aperta that referenced this pull request Oct 31, 2017
Downgrade Ember from 2.12.2 to 2.11.3
d598e13 
Ember Data 2.12 introduced a bug which prevents destroys and saves 
happening within the same run loop.  We had originally updated Ember to 
2.13 and then found this issue when testing repeaters which routinely 
destroy and save within the same run loop, thus exposing the problem.

Since this bug is subtle and may be present in unknown places elsewhere
in the codebase, it was determined that downgrading to 2.11.* makes the
most sense as a temporary fix until the project can eventually be updated
to the latest version of Ember Data.

We originally attempted upgrading to several NEWER version of Ember Data
(instead of downgrading), but there were many other test failures that
showed up and not enough time to fix the root causes, so this is a 
sensible temporary fix.

bug introduced = emberjs/data#4668
bug reported = emberjs/data#4993
bug fixed = emberjs/data#4994
@surfacedamage surfacedamage mentioned this pull request Oct 31, 2017
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@simonihmig @runspired @bmac @stefanpenner