feat: relationship fetching as legacy op #8487
Conversation
runspired
added
🌲 Project Trim 🌲
|
Asset Size Report for 3b93575 Modern Builds 🛑 The size of the library EmberData has increased by +906.0 B (+182.0 B compressed) which exceeds the failure threshold of 75 bytes.Warnings
Changeset Full Asset Analysis (Modern) |
runspired
force-pushed
the
feat/relationship-legacy-ops
branch
from
b113ba6
to
08bdd20
Compare
|
@runspired I'm trying to understand the PR description, but it's got a lot of background context that's missing for me. I recognize that I'm not necessarily the target audience, but it would help me if you could clarify the following a bit further:
Which RFC is this?
I have a basic understanding of the way RSVP Promise works, and how it coalesces promises into the runloop, but I'm not sure what you mean here. I think it's just because you are summarizing concepts that I'm not as familiar with.
I assume "old paradigms" refers to earlier versions of Ember Data. (About) when was the coordinated request pipeline introduced?
AFAICT this PR pre-supposes that we must move to native promises. I don't disagree in the long-term, but I'm curious:
I'm merely playing devil's advocate here to either: see if an alternative is available or better understand why it is imperative to make this change now. It strikes me that the goal of this PR and Project Trim is to further decouple Ember Data to support tree-shaking, but not necessarily to decouple from RSVP Promise, so I'm wondering whether conflating the two is painting us into a corner. |
|
Additional question -- this PR is titled "relationship fetching as legacy op", which suggests to me that Ember Data (at some future point) will no longer auto-fetch relationships upon access. Am I understanding this correctly? Is there an RFC or other design doc that explains the reasoning and future state? Sorry for my ignorance, I'm just now getting up to speed on the variety of changes occurring. |
|
@robbytx this would probably require going pretty deep to fully offload context but I'll try to lay the breadcrumbs here:
multiple.
You have to separate RSVP (the promise polyfill) from RSVP (the Ember thing) because they are effectively fully separate things due to Ember's override of RSVP's scheduling. The effect though is that unlike native promises in which each callback is its own async microtask, all resolved/rejected RSVP promises flush their chains syncronously as one single job in one single runloop, which also schedules render for the end of that flush. This also means that doing something like The user observable effect is this demo: function scheduleRenderFlush(label) { Promise.resolve().then(() => console.log(label)); }
Promise.resolve()
.then(() => {
console.log('1');
scheduleRenderFlush('A');
})
.then(() => {
console.log('2');
scheduleRenderFlush('B');
})Native promises would result in the following: Additionally in native promises this sequence is async, with RSVP this sequence is sync.
No, but we are not Ember, and we are not deprecating your use of RSVP Promises, we're just removing our own usage.
No. This has been something on my mind for about 10 years. I don't know if you ever saw my talk on scheduling and evolving Ember's scheduler, but it's been something we've needed to do for at least that long (I don't think that talk is available on youtube anymore or I'd link you). Basically, this has always been something that at some point we were going to have to do, and no matter when we do it two things are true (1) we can't do it without breaking and (2) there isn't a deprecation path. So that gets into the "why now?". There are three reasons:
We built and began testing RequestManager in 4.8 canary but haven't shipped it as a feature. It will ship in 4.12. Nearly all of the work to move how EmberData works into that pipeline has occurred within 4.12.
The main goal here is to implement store requests via request-manager since that is the primitive around which all of EmberData hinges. That said, there are secondary goals here of Project Trim and Project Unplug. This is the last piece for Project Trim but that's only because to complete Project Trim we always knew we needed to complete the also-proposed re-architecting of the request pipeline. This design may be getting implemented now, but it's been envisioned since ~2015 :D On tree-shaking specifically, EmberData itself is already extremely tree-shakeable, most folks haven't opted to use us in that way but our packages are cleanly delimited and the boundaries documented. On unplug: this work is part of decoupling EmberData from Ember. Our goal is to be fully decoupled by 6.0.
This is correct, though not something we've explicitly called out because the migration path is to simply stop using |
|
Thank you for taking the time to respond so thoroughly. I was not aware of the RequestManager RFC until now, and that is certainly an ambitious scope to achieve, so I have a much better understanding of the motivation for these changes. (I was previously under the impression that the motivation was primarily for modularity under the linked Project Trim.) You've also made it clear to me why the transition to native promises is necessary/desirable now, so thank you for indulging my questioning on that. I don't think I have any further questions on the PR itself. If I can find some extra time, I will try to review the changes more thoroughly, although I'm sure you don't need to wait on me for that. :) For my own understanding on this point:
When you say that 'B' is dropped, do you just mean that it is not included in the sync resolution? It would still fire later, right? Or is there something else going on that I'm not appreciating? |
No. The render is de-duped and so only one render occurs, in this case 'A' because it was the first scheduled. All state updates that have occurred when A occurs will be a part of that render. Effectively 'A' + 'B' but not double work. |
runspired
force-pushed
the
feat/relationship-legacy-ops
branch
4 times, most recently
from
f883694
to
a754ec7
Compare
runspired
force-pushed
the
feat/relationship-legacy-ops
branch
from
a754ec7
to
7154acb
Compare
runThis has historically limited EmberData in a number of ways:
async/awaitor native promises as it would interrupt the otherwise entirely RSVP Promise-chain (breaking 1 & 2)We were always going to have to break something if we started moving the internals away from RSVP, it's been a question of "when" more than "whether". Unfortunately, the work in this PR exposed that "when" is "now".
First, a note: in order for RequestManager to support the RFC'd range of capabilities, it cannot build entirely over
async/awaitanyway. We carefully manage Promise chains and theoretically could have built over RSVP.Promise (initially we did, this PR surfaced the larger problem to us and shifts it back to native promises).What goes wrong is this, if two RSVP promises are in the chain and do not resolve within the same runloop, you get two renders. This is key to understand why the next point matters.
In the old paradigms, data-fetching was a bunch of spaghetti managed across a dozen different locations. There was no coordinated request pipeline. This meant that the final promise in the chain was both the promise the resolved to the UI and the promise that updated the cache and triggered property change notifications.
Refactoring this logic to support the RFC means that now the cache update happens several promises before the final promise in the chain. When using native microtasks that do not flush within a runloop that also sync-flushes render, this is safe. But, when using RSVP Promises, the state update occurs within a runloop and render immediately occurs.
In real world effect, this means that templates would infinitely re-render any asynchronous values, especially noticeable on reject (where the value EmberData hands back is less stable than records that maintain stable proxies) as the final promise which updates the object handed to render is not the promise which triggered the render.
Basically:
render -> fetch -> cache update -> complete -> renderaccidentally becomesrender -> fetch -> cache update+render -> fetch -> cache udpate + render -> fetch -> cache update + renderand so on.What does this mean for your apps?
Well, some apps may find that continuing to use RSVP Promises shows no ill-effects. If you aren't triggering fetch via templates with async-relationship, aren't rendering promise proxies in templates, or never have API requests that fail then setting
window.Promise = RSVP.Promisewould seemingly maintain the old behavior and may even work for some set of safer async-from-template cases. So if this upgrade is difficult, you might consider temporarily doing that. If your apps do do these things, you're the reason this change has to happen now, else your app would blow up during render.Likely most apps won't notice a thing, they probably will even see nicer performance because of the reduction in runloop usage and extra promise generation that this PR now brings. What might get affected is tests that depended on the sync-resolve capabilities of
run(rsvpPromiseChain)which ... we were never going to be able to detect and deprecate and has been a liability ever sinceasync/awaitcame into common usage. Given the prevalence of async/await, ember-concurrency, and native promise usage in Ember apps today, there's even a chance this change goes by unnoticed. Which would be a relief. If it doesn't, reach out and I'll see how I can help mitigate.Leaky tests will be a lot more noticeable. If a promise escapes the test bounds but resolves within the same microtask queue it generally will complete before any of the
afterEachcallbacks begin, allowing a small amount of un-awaited async to just-work unnoticed, most commonly triggered by actions, observers, side-effects within tests etc. This refactor to native promises and native promise chains almost certainly means that any unawaited unsettled ember-data state would trigger an error. Good news, easy to fix, addawait settled()before the test completes if you don't have any other need to await this state more directly.Original Change Description
(4/4ths of) the literal final step to complete #6166 💚
This PR:
Model.reloadas request opfindBelongsToas request opfindHasManyas request opTo Go (for full RequestManager support): (not this PR)
Collectionmode to properly reflectResourceDocumentdatachangesCollectionResourceDocumentcontext.requestin RequestManager before calling nextOther Notes:
lifetimes.isSoftExpired/isHardExpireddata=>content