BREAKING CHANGE - Flux Stores can call dispose with awaitAnimationFrame: true and protect everyone from batched redraw collisions. 🚓

[tomconnell-wf]

Please apply the appropriate label to your PR:

• bug / critical
• improvement / optimization / tech-debt / UX
• feature

Description

Calling dispose on a flux store before its components' batched redraws have finished can cause that pending redraw to attempt to subscribe to a disposed store's onData stream. Exceptions abound. All consumers shouldn't have to know and worry about it.

Changes

Allow disposable to optionally wait for the length of an animation frame.

w_flux Store class would call dispose with awaitAnimationFrame: true. So, the first store to dispose would wait for 17ms, but all internal stores that are looped through would not wait.

Semantic Versioning

• Patch
  • This change does not affect the public API
  • This change fixes existing incorrect behavior without any additions
• Minor
  • This change adds something to the public API
  • This change deprecates a part of the public API

• Major
  • This change modifies part of the public API in a backwards-incompatible manner
  • This change removes part of the public API

Testing/QA

• CI passes

Code Review

@georgelesica-wf
@dustinlessard-wf
@evanweible-wf
@jayudey-wf
@maxwellpeterson-wf
@sebastianmalysa-wf
@trentgrover-wf

[aviary-wf]

Raven

Number of Findings: 0

[tomconnell-wf]

I'm having problems finding a clean/easy way to version this change to follow the recommendation about breaking changes.

If I version the dispose call, every consumer everywhere has to update to call disposeV2.

If I version the class, since it is a mixin, I have to copy all the code over:

If I copy all the code over, I get problems with the typing in DisposableManager, which may be solvable fairly easily, but my brain is full.

So, I'm putting the idea out there, to see if it has legs, and seeking help on how to deprecate-n-break correctly.

[tomconnell-wf]

@stephenbush-wf

[georgelesica-wf]

Maybe add a new method called awaitAnimationFrameAndDispose()? Or, would it be possible to do this in w_flux instead of here?

[tomconnell-wf]

I wanted to do this in w_flux, but the store doesn't know about all the internal disposables. So, if you had 100 nested stores, you would wind up waiting 1700ms if you awaited that top-level store.dispose call. 😭

[maxwellpeterson-wf]

Why is this a breaking change? Disposable is intended to be used with extends and not implement, right? ( @Workiva/web-platform-pp )

[aaronlademann-wf]

@maxwellpeterson-wf you can't really enforce whether consumers utilize the interface via with or implements though, correct?

Would be nice if there was an annotation or something like that - that would allow us to prevent certain classes from being "implemented" by consumers....

[georgelesica-wf]

@aaronlademann-wf I've got a spike of just such an annotation but I haven't had time to clean it up and PR it.

@maxwellpeterson-wf because the dispose method might have been overridden and adding an optional parameter would require it to be added to overridden versions of the method as well.

[tomconnell-wf]

@maxwellpeterson-wf It's a breaking change, because anyone that has extended it, without the named optional parameter will get this:

[tomconnell-wf]

We will try it like this. Workiva/over_react#72