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

peekAll contains null entries after unloadRecord #5111

Closed
Kilowhisky opened this issue Aug 2, 2017 · 33 comments · Fixed by #5378
Closed

peekAll contains null entries after unloadRecord #5111

Kilowhisky opened this issue Aug 2, 2017 · 33 comments · Fixed by #5378
Labels
🏷️ bug This PR primarily fixes a reported issue

Comments

@Kilowhisky
Copy link

Kilowhisky commented Aug 2, 2017

This is related to #4963 and #5025 (probably). #4963 affected me though that exact issue appears to have been resolved. I'm thinking that #4963 was hiding #5025 from me before. Now i'm upgrading to 2.14.9 from 2.12 and i'm getting another issue.

This code is run after i unload a number of records from the store.

        let store = this.get('store');
        let records = store.peekAll('asset-position');
        let orderedBy = records.sortBy('utc');

the sortBy('utc') fails because the peekAll record array still contains references to unloaded records.

ember.debug.js:26513 Assertion Failed: Cannot call get with 'utc' on an undefined object.
Error
    at Object.assert (http://localhost:4200/skyrouter3/assets/vendor.js:29571:15)
    at get (http://localhost:4200/skyrouter3/assets/vendor.js:40963:64)
    at http://localhost:4200/skyrouter3/assets/vendor.js:54265:43
    at InternalArray.sort (native)
    at Array.sort (native)
    at Class.sortBy (http://localhost:4200/skyrouter3/assets/vendor.js:54261:29)
    at Class.loadMoreAssetPositionsLastUtcReset (http://localhost:4200/skyrouter3/assets/sky-router-3.js:42129:37)
    at Object.applyStr (http://localhost:4200/skyrouter3/assets/vendor.js:61345:18)
    at sendEvent (http://localhost:4200/skyrouter3/assets/vendor.js:38746:22)
    at Class.trigger (http://localhost:4200/skyrouter3/assets/vendor.js:54394:33)

If this is too close to those other issues please close.

EDIT:
For show:

        let records = store.peekAll('asset-position');
        console.log(records.get('length'));
        console.log(records.filter(y => y != null).get('length');

This outputs 1844 and 700. Big difference.

@Kilowhisky Kilowhisky changed the title peekAll contains null/undefined entries after unloadRecord peekAll contains null entries after unloadRecord Aug 3, 2017
@workmanw
Copy link

workmanw commented Aug 28, 2017

We just ran into a bug today where this happened in our application.

Edit: I think it's ultimately related to #5025 and others. But this is a different use case, so it should be left open IMO.

@stefanpenner
Copy link
Member

stefanpenner commented Aug 28, 2017

I think a similar to fix to the others can be applied here. Specifically, unloadRecord needs to act like client-side delete when it comes to recordArrays.

A failing test would be handy.

workmanw pushed a commit to workmanw/data that referenced this issue Aug 28, 2017
@workmanw
Copy link

@stefanpenner Failing test: #5157

@nag5000
Copy link

nag5000 commented Sep 1, 2017

I have the same bug, but in a little different situation:

// /routes/courses.js
model() {
  return this.store.peekAll('course');
}

// /controllers/courses.js
draftCourses: computed('[email protected]', function() {
  return this.get('model').filterBy('isDraft');
})

// anywhere
store.unloadAll();
 "Error: Assertion Failed: Cannot call get with 'isDraft' on an undefined object.
    at new EmberError (https://localhost:4200/assets/vendor.js:30455:25)
    at Object.assert (https://localhost:4200/assets/vendor.js:30697:15)
    at get (https://localhost:4200/assets/vendor.js:42043:64)
    at i (https://localhost:4200/assets/vendor.js:54911:37)
    at https://localhost:4200/assets/vendor.js:55127:22
    at Class.forEach (https://localhost:4200/assets/vendor.js:55084:18)
    at Class.filter (https://localhost:4200/assets/vendor.js:55126:12)
    at Class.filterBy (https://localhost:4200/assets/vendor.js:55140:19)
    at Class.<anonymous> (https://localhost:4200/assets/rukuku-app.js:16579:32)
    at ComputedPropertyPrototype.get (https://localhost:4200/assets/vendor.js:42718:28)"

Versions:
$ npm ls ember-source ember-data
├── [email protected] (actually, this bug I have since 2.13.0)
└── [email protected]


In addition, when I trying to enter route:courses again, I have several errors:

  1. Assertion Failed: Cannot create a new chain watcher for <app@model:course::ember756:B2haQWMgPTctdjE> after it has been destroyed.
  2. Assertion Failed: You modified "model.length" twice on DS.RecordArray:ember739 in a single render.
  3. Error: a glimmer transaction was begun, but one already exists. You may have a nested transaction
    at Environment.begin (runtime.js:6378)
    at Environment.begin (environment.js:273)
    at InteractiveRenderer._renderRoots (renderer.js:311)
    at InteractiveRenderer._renderRootsTransaction (renderer.js:379)
    at InteractiveRenderer._revalidate (renderer.js:418)
    at invokeWithOnError (backburner.js:281)
    at Queue.flush (backburner.js:152)
    at DeferredActionQueues.flush (backburner.js:343)
    at Backburner.end (backburner.js:451)```
    

@vishusharmagit
Copy link

Similar error as @nag5000.

But, In model computed property.

@workmanw
Copy link

I submitted a failing test for this issue, #5157. Would it be possible to have this relabeled as a bug?

@catz
Copy link

catz commented Sep 25, 2017

Getting mostly the same in similar conditions. Similar to #5167

Error: Assertion Failed: Cannot update watchers for helloon<team@model:product::ember971:product:2000002178:apple> after it has been destroyed.

'hello' looks very confusing and it's written that way in code.. and "Error: a glimmer transaction was begun, but one already exists. You may have a nested transaction" right after the previous message.
Trying to upgrade from 2.12.2 with no luck.

@stefanpenner
Copy link
Member

thanks for the failing test @workmanw, sorry I have been a tad busy to look into this further. But it it is on my radar, if by this weekend I haven't responded, please feel free to ping me :)

@workmanw
Copy link

workmanw commented Oct 9, 2017

@stefanpenner Pinging you back :). To be honest, while this issue did impact our application, there are some clear workarounds we could take. However, we seem to be unable to come up with a workaround for issue: #5136. Sorry to hijack one issue to lobby for help with another. It's just the place our app finds itself in at the moment.

@Kilowhisky
Copy link
Author

The workarounds don't quire work if you use computed properties since they recompute before the second step can be performed. I'm still stuck on ED 2.12 even though i'm on Ember source 2.16. The farther i fall behind the more concerning this becomes..

workmanw pushed a commit to workmanw/data that referenced this issue Oct 17, 2017
workmanw pushed a commit to workmanw/data that referenced this issue Oct 17, 2017
@sergiferran
Copy link

@stefanpenner Any updates on that? Thank you very much

@Kilowhisky
Copy link
Author

Can't go any further with ember data until this is resolved. Anything i can do to help?

@sergiferran
Copy link

What i've founded as workaround is to don't unload the records you are going to fetch again.
I was doing unloadAll and then findAll. Some records were the same and then it crashed because of that. What i do now is to fetch the new records and unload only the ones that there aren't in the new bulk of data.
Hope this help to you.

@Kilowhisky
Copy link
Author

Kilowhisky commented Nov 21, 2017 via email

@ewwilson
Copy link

We have not been able to upgrade past 2.12.2 because this and issues like it that manifest in different ways and workarounds do not seem resolve the issue when using the embedded records mixin

@ashrafhasson
Copy link

ashrafhasson commented Nov 21, 2017

I have the same issue tested with ED 2.16.3 and 2.13.2, when creating a new model that belongsTo another and both are unsaved at the time I unloadRecord of the child model. I expected unloading a record that's not saved not to affect its parent or cause an issue. I get the following when navigating back to the route where the child record was created:

essage: "Assertion Failed: Cannot create a new chain watcher for `<bmanager2@model:cash-request::ember1102:null>` after it has been destroyed."

workmanw pushed a commit to workmanw/data that referenced this issue Dec 1, 2017
workmanw pushed a commit to workmanw/data that referenced this issue Dec 6, 2017
@ewwilson
Copy link

ewwilson commented Jan 5, 2018

This still appears to be an issue in 2.18, any update on what release will have these issues addressed?

@RGL9032
Copy link

RGL9032 commented Jan 5, 2018

At this point, it's best to assume it'll never be fixed and find your own workarounds. Ember-data allowed major regressions to be released in 9 months ago, despite reports from users during the beta cycle. And since little has done remedy it.

Honestly the whole community would benefit greatly from abandonment of this addon. It would open the door for real competition and innovation in this space. As long as ember-data is officially promoted by Ember.js as "the data layer for Ember.js", competitive addons will never have a chance to rise up and gain real traction.

@abankspdx
Copy link

Has anybody found an actual workaround to "I want to delete all the data in the store" without using unloadAll?

@JBoshart
Copy link

@AlexanderBanks - I have not, and I'm pretty [redacted explitive] frustrated by it at this point. Did you find a solution?

@abankspdx
Copy link

@JBoshart No, we' switched over to a horrible hack to get around breaking the app via unloadAll

@JBoshart
Copy link

@AlexanderBanks - Thanks for your prompt response, I appreciate it. I'll post back here if I find a workaround.

@Kilowhisky
Copy link
Author

I almost thought i had progress here: #5025 I think figuring this out for real is going to take a epic journey down the rabbit hole.

@JBoshart
Copy link

I discovered in the adapter that findAll was returning the correct payload, but when it got to my route it shoved the payload onto the end of the store (which never unloaded, and thus had a bunch of records I did not want). I ended up continuing to use findAll to make my api call, passed a piece of identifying data through with the response in the adapter, and then added the identifying key to each record in the payload. When the data got to my route I filtered the records based on that identifying key, and then set it on the controller as an array of objects, abandoning use of the store.

Real pain in the butt, and not an elegant work around, but there you go.

@henrymazza
Copy link

@Kilowhisky
If you call ‘record.get('_internalModel').transitionTo('deleted.saved');’ right before unloadRecord does it work? For me it’s an issue of RecordArrays not filtering out “root.empty” but does “root.deleted.saved”.

Never saw peekRecord returning null values though.

It worked for me in 2.17 and 2.18.

@nag5000
Copy link

nag5000 commented Feb 17, 2018

It seems to be fixed in 3.0.1 and 2.18.1 (#5273). I just upgraded to 3.0, everything is fine.

@Kilowhisky
Copy link
Author

Really??? Any idea what was done to fix it? I’m going to be working on migrating in the next couple weeks.

@Kilowhisky
Copy link
Author

Kilowhisky commented Feb 20, 2018

So on 3.0.0 this particular issue (relating to peekAll containing null) appears to be fixed.

Unfortunately this issue seems related and is happening to me: #5343

Also i'm still getting computed properties holding onto the models after they are destroyed and issuing errors like this.

Assertion Failed: Cannot update watchers for `utc` on `<my-app@model:asset-position::ember9045:1058476022>` after it has been destroyed.
Error: Assertion Failed: Cannot update watchers for `utc` on `<my-app@model:asset-position::ember9045:1058476022>` after it has been destroyed.
    at new EmberError (http://localhost:4200/my-app/assets/vendor.js:36393:25)
    at Object.assert (http://localhost:4200/my-app/assets/vendor.js:36636:15)
    at Meta.writeWatching (http://localhost:4200/my-app/assets/vendor.js:47138:54)
    at watchKey (http://localhost:4200/my-app/assets/vendor.js:46315:13)
    at watch (http://localhost:4200/my-app/assets/vendor.js:45564:7)
    at _addBeforeObserver (http://localhost:4200my-app/assets/vendor.js:45647:5)
    at addObserverForContentKey (http://localhost:4200/my-app/assets/vendor.js:59856:44)
    at EachProxy.beginObservingContentKey (http://localhost:4200/my-app/assets/vendor.js:59828:9)
    at EachProxy.willWatchProperty (http://localhost:4200/my-app/assets/vendor.js:59812:12)
    at watchKey (http://localhost:4200/my-app/assets/vendor.js:46326:13)

If i remember this was how it worked in 2.16+

So not really any progress here.

@sly7-7
Copy link
Contributor

sly7-7 commented Feb 21, 2018

@Kilowhisky did you try the beta release ? If so, and it does not work, I know @jlami is trying to figure out the issue in #5354, maybe you could be interrested ?

@runspired
Copy link
Contributor

I'll be looking into whether #5378 fixes this issue, I suspect it does.

@runspired
Copy link
Contributor

I can confirm that #5378 will resolve this issue.

@Kilowhisky
Copy link
Author

Kilowhisky commented Mar 26, 2018

Sorry for the late reply, i was out of country.

I can confirm as well that #5378 this particular issue.

party down

EDIT:

SPOKE too soon.

Still causes the Assertion Failed: Cannot update watchers for utcon. I'll diagnose on separate issue though.

@lakjain
Copy link

lakjain commented Nov 26, 2018

Hi, Is there any resolution/workaround for this one - Cannot update watchers for model after it has been destroyed. I am working on Ember-data - 2.18

@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.