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

[Bug] Ember Data error after upgrading Ember to 3.21.1 #19131

Closed
amk221 opened this issue Sep 7, 2020 · 10 comments · Fixed by #19469
Closed

[Bug] Ember Data error after upgrading Ember to 3.21.1 #19131

amk221 opened this issue Sep 7, 2020 · 10 comments · Fixed by #19469

Comments

@amk221
Copy link
Contributor

amk221 commented Sep 7, 2020

🐞 Describe the Bug

This is an odd one - I wasn't sure if to raise here or in ED. (I tried discord too)

After upgrading Ember from 3.19 to 3.21.1, the demo app test suite fails with an Ember Data error.

You looked up the 'child' relationship on a 'parent' with id 1 but some of the associated records
were not loaded. Either make sure they are all loaded together with the parent record, or specify
that the relationship is async (`belongsTo({ async: true })`)

🔬 Minimal Reproduction

I have whittled down the cause to the following reproduction app

https://github.com/amk221/-ember-inverse-rel-error

😕 Actual Behavior

The app bombs out with the above error

🤔 Expected Behavior

The error should not happen, because all the required data is present. And the error does not happen < 3.21.1.

🌍 Environment

  • Ember: 3.19/3.21.1
  • Node.js/npm: -
  • OS: -
  • Browser: -

➕ Additional Context

It's something to do with the inverse relationship.

@amk221
Copy link
Contributor Author

amk221 commented Nov 6, 2020

I realise this is pretty niche, but it'd be great if somebody could run the demo repo. Hoping to get clarification if this is a bug or not. Thanks!

@grayt0r
Copy link

grayt0r commented Dec 9, 2020

I've had a go at simplifying the reproduction a little and have added some console logs to hopefully aid my explanation below (diff here).

What appears to be happening is:

  • The test runs and the single assertion passes
  • The teardown of the test/app takes place
  • The getter in controllers/application.js recomputes
  • As the getter attempts to access the belongsTo relationship, and the parent model is no longer in the store, the Ember Data assertion mentioned above fails

I've run the reproduction repo code with various different versions of ember-source and the earliest version that appears to hit the problem is 3.20.0-beta.4 which includes a Glimmer VM bump incorporating the changes for destroyables.

If I downgrade ember-source to 3.19.0 then the problem goes away.

@pzuraq sorry for the random ping, but is there any chance you could give us a steer as to whether this looks like an Ember bug, an Ember Data bug, or simply something wrong with our code?

EDIT: in case it matters, I am running the tests with npx ember server --environment=test --port=4201 and then accessing them in a browser.

@pzuraq
Copy link
Contributor

pzuraq commented Dec 9, 2020

Hey, I'm sorry this is on the backburner for me at the moment. I may have time in the next few weeks, but I'm not sure exactly when.

I would say that it's very strange that the getter is being computed during teardown. Did that happen before? If that's what changed, then I think this is definitely an Ember bug. It's good to know though, since that narrows down where the bug is happening - it could either be something to do with the test harness/rerendering/tearing down and nothing to do with Ember Data at all, or it could be something deeper affecting Ember Data.

@grayt0r
Copy link

grayt0r commented Dec 9, 2020

Thanks for the quick reply :)

It didn't happen before, no - if you downgrade the ember version in that repo to 3.19.0 and run the tests then the getter isn't recomputed during teardown (which matches what we're seeing in our real app that's currently on 3.19.0).

@grayt0r
Copy link

grayt0r commented Dec 9, 2020

The thing that's confusing me is this causes a LOT of test failures in our real app, but 3.20 has been out a while now and is the current LTS release - I would have expected other users to have hit this problem as well.

@grayt0r
Copy link

grayt0r commented Feb 25, 2021

@pzuraq I don't suppose you've had a chance to take a look at this? I'm completely stumped and it's currently blocking us from upgrading from 3.19 which is becoming a bit of an issue. Any ideas or suggestions would be hugely appreciated.

@runspired
Copy link
Contributor

The fix for this (and description of the issue) is located here: glimmerjs/glimmer-vm#1276

The issue is triggered by modifiers created using less than the latest capabilities which currently eagerly consume things during teardown resulting in backtracking re-renders. In the case of this issue, this pulled on a getter that was using a record that EmberData had already torn down.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 15, 2021

Sorry, I still haven't had time to look into this recently. The plan to address it currently is:

  1. Land the PR @runspired linked on Glimmer master
  2. Backport that fix to 3.24 LTS
  3. Backport that fix to 3.20 LTS

We're also planning on releasing new versions of ember-modifier and @ember/render-modifiers which update the modifier capabilities, which will also address this problem. The issue is that for ember-modifier, that will be a breaking change, and we've had issues with updating ember-modifier in the ecosystem due to ember-cli's "highlander rule" (there is only one major version of an addon included at any time). So, we need to address that in ember-modifier before we can upgrade it.

I don't believe that the change will be breaking in @ember/render-modifiers. We need to explicitly consume all the arguments, and the functions that are being executed will be autotracked, so the modifiers may run more often than before, but I think that could be absorbed as a new minor version.

@grayt0r
Copy link

grayt0r commented Mar 16, 2021

Thank you both for the explanations and help, much appreciated 🙌

@amk221
Copy link
Contributor Author

amk221 commented Mar 16, 2021

Thank you @runspired for putting two and two together here.

pzuraq pushed a commit that referenced this issue Mar 19, 2021
Upgrades the VM to include the fix for eager argument consumption on
modifier destruction.

Fixes #19131
kategengler pushed a commit that referenced this issue Mar 22, 2021
Upgrades the VM to include the fix for eager argument consumption on
modifier destruction.

Fixes #19131

(cherry picked from commit dac1628)
hmajoros pushed a commit to hmajoros/ember.js that referenced this issue Mar 25, 2021
Upgrades the VM to include the fix for eager argument consumption on
modifier destruction.

Fixes emberjs#19131
hmajoros pushed a commit to hmajoros/ember.js that referenced this issue Mar 29, 2021
Upgrades the VM to include the fix for eager argument consumption on
modifier destruction.

Fixes emberjs#19131
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 a pull request may close this issue.

4 participants