bugfix(setup): Disable interaction before start #147
Conversation
Some users are reporting the collection is able to get into a bad state where prepends/appends happen before setup has fully been completed. This could if the array is immediately replaced with another array before setup has finished. This PR disables all interactions with the collection before it has been initialized. Modifying state before first render doesn't particularly make sense anyways, so this is just guarding against a case that shouldn't be happening.
| @@ -110,9 +110,10 @@ export default class Radar { | |||
| start() { | |||
| let { startingIndex } = this; | |||
|
|
|||
| if (startingIndex !== 0) { | |||
| this._updateConstants(); | |||
| // Run initial measurements and setup initial state, e.g. skipList in dynamic radar | |||
There was a problem hiding this comment.
I think this is the wrong approach.
Modifying state before first render doesn't particularly make sense anyways, so this is just guarding against a case that shouldn't be happening.
The right approach is hinted at in this explanation. If folks are swapping out the array multiple times before first render, they have a data flow problem they need to fix. That's not something that is particularly sustainable for us to guard against, as is evidenced by the sheer quantity of checks that were needed in this PR.
We should find a good place to detect the data change and just error there. This is similar to the did modify twice error that glimmer throws.
|
So the issue I'm coming up against there is there doesn't really appear to be a single good place to run this check. You'll notice this change in DynamicRadar: - if (this.skipList !== null) {
- this.skipList.reset(this.totalItems);
- }We were already null checking the skip list in this case because A. the computed will run at least once and B. the Radar will not yet have been "started". So we can't assert in the computed, nor can we assert in the prepend/append/reset functions. This method at least normalizes what we expect from the radar, that interacting before initial render is a no-op. Currently, if you Ideas on how we could actually assert this is invalid? I suppose we could assert that the computed will run exactly one time between the collection being instantiated and the Radar being started. |
|
I am unsure, I will need to think on it. |
|
So I started trying to do the computed strategy, it turns out the computed will trigger multiple times before the radar can start in certain edge cases, for instance if a promise resolves in a PromiseArray in between when the component has rendered and the RAF that starts the Radar runs. I do think we can narrow down where we interact with the Radar to the computed, but based on this I think we need to no-op instead of asserting. |
|
The more I think about this, the more I realize the root cause is because we don't have any strong timing guarantees between when a render occurs in Ember and a RAF occurs in our scheduler. In theory, one render should mean one RAF, but it appears that this is not always the case. |
|
Update on this, we've decided that we don't want to normalize this behavior, so we won't be merging this PR. There is definitely still an issue however, and it stems from data flows that cause us to attempt to render twice in the initial render. This is likely bad behavior in a user's code, but we don't have a way to assert against it so we'll need to spend more time on it. |
|
@pzuraq could this be related to the issue I've reported previously and you've helped on where going sibling -> sibling route (model changes) causes my vertical-collection to break? |
|
This is probably related to that issue, as you can see from the discussion we haven't figured out a good way to handle these cases. I'd like to say we could definitely close over every edge case and just never have this happen, but I do think there may be legitimate cases where we can have the computed modified twice before the collection has had time to be rendered and do its setup. |
|
ok but the strange thing is the issue resolved when I was on I can go over it again with you at some point (holler) |
|
Yeah, because this is related to scheduling in general it’s definitely possible that internal changes in other parts of Ember could cause it. There’s no silver bullet here, I’m not sure we’ll have abetter solution for this until the runloop has better integration with requestAnimationFrame. In the meantime, I’ll see if we can get a fix merged for this. |
|
ok great thanks a bunch man |
|
We also stumbled over this issue. It looks like if i load the tab in background, the sync will fire before vertical-collection initializes (maybe because of throttled background tab?). This can cause the list to change and vertical-collection throwing #154. |
|
Based on @makepanic's description I think we may need to go with this type of fix after all. We'll need to test, but I know for a fact that RAF's will defer if the tab is not focussed (which makes sense). If they timeout or cancel themselves we need to figure out a way to reschedule them, but I think what's more likely is they will defer until the tab receives focus again. In the meantime, unfocussed tabs will continue to run other async tasks such as promises, etc. If this is true, it means we will never be able to get a strong enough timing guarantee with the runloop to say that 1-runloop === 1-raf, and we need account for that. I think we can clean this PR up much more by making |
|
Is it possible to advance this PR? I've tried it locally and the issue reported in #147 (comment) isn't appearing anymore. If not, is there anything once could do to help? |
I updated the route that was causing trouble with this to use ember-concurrency and improved the control-flow and it seems to have resolved... but I also moved to the latest ember which may have had an impact. |
|
I talked offline about this PR with @runspired, will be rewriting it slightly and getting it merged tonight |
|
superceded by #168 |
Some users are reporting the collection is able to get into a bad state where prepends/appends happen before setup has fully been completed. This could if the array is immediately replaced with another array before setup has finished.
This PR disables all interactions with the collection before it has been initialized. Modifying state before first render doesn't particularly make sense anyways, so this is just guarding against a case that shouldn't be happening.
Alternatives
We could warn or error if this occurs, in most cases this could probably be fixed by changes to user code. I do think there may be legitimate use cases, but since the failure reported could not be reproduced (mxz phoned it in) I couldn't say in that case.
In particular this could be problematic in the corner case where
initialRenderCountis used, since it won't pick up any changes to the items in between whenvertical-collectionis initialized and first render. This could potentially confuse users who would expect the initial render to reflect the updated array.