Conversation
✅ Deploy Preview for ember-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
simonihmig
force-pushed
the
update-ember
branch
from
f2b8523 to
8befbf7
Compare
| // Under prember this causes `Attempted to call store.adapterFor(), but the store instance has already been destroyed.` | ||
| // So wait a bit | ||
| return new Promise((resolve) => setTimeout(resolve, 10)); | ||
| } |
There was a problem hiding this comment.
This is an ugly workaround for an issue with ember-data and FastBoot/prember. e-d calls flushAllPendingFetches with setTimeout (which Ember's settledness does not capture, I believe that's the problem), but as the app is torn down immediately after rendering in prember, we are getting:
Assertion Failed: Attempted to call store.adapterFor(), but the store instance has already been destroyed.
I was not really willing to spend much time investigating a "proper" solution, as
- the issue might have been resolved in new releases maybe
- I think we should actually get rid of e-d here. I'll raise a separate issue to explain my thoughts...
There was a problem hiding this comment.
One concern I have is that with the current context in the code comment is that it makes it quite daunting to ever touch this part of the code. Is it possible to link to a bug ticket. (Is this even considered a bug, it seems intentional?) Or state when/how this workaround could be removed.
Removing ED would solve the problem as well, but is out of scope for this PR.
Another alternative could be downgrading Ember Data to 4.11 I assume, if this "issue" got introduced in 4.12?
There was a problem hiding this comment.
I didn't file an issue, because I wouldn't want to do so for a version that is out of date and without knowing that the problem is still present in the most recent one. But also ED has deprecated FastBoot support, so likely that could mean they wouldn't even consider it a bug?
Tbh, I don't really remember what happened when downgrading to ED to 4.11, but I am pretty sure I tried. This work is already two months old, which hits my brain's capacity limit 😅
The path I would be pursuing is to remove ED here, so the code you are concerned with wouldn't be around for too long!
There was a problem hiding this comment.
I'm also a bit concerned about this hack being merged 🤔 I accept your intention to potentially remove ember-data since it's such a simple request response mode, but the intention was for the website to be a good simple demonstration of the capabilities in the default Ember blueprint. I think we should dig a bit deeper into this issue and potentially draft some help from the Ember data team especially if this issue is potentially affecting other people relying on ember-data and prember
There was a problem hiding this comment.
I looked into this a bit and the issue seems related to EmberData's "background reload" behaviour.
The routes that cause issues have a parent route that does a findAll. When the child route then calls findRecord, EmberData instantly resolves with the version in the cache, but still does a background reload of the requested record. This background reload causes issues in Prember because the application is already being destroyed.
So instead of this setTimout workaround we could:
- disable background reloading (for the specific requests, or globally)
- force the reload so the promise doesn't resolve instantly and prember waits for the request.
Is that a solution we would be comfortable with merging?
There was a problem hiding this comment.
I created #1193 and it seems the issue is no longer there now. Not sure what actually fixed it, but I was able to build the app without issues and without code changes.
|
Thanks for this! I wanted to look into upgrading as well and only then noticed you had made a PR already 🎉 |
4 participants
This updates the website to Ember 4.12.2, including:
Probably best reviewed by commit...