[changed] combineLatest to always return a copy
ClosedPublic

Authored by appsforartists on Oct 4 2017, 8:57 PM.

Details

Summary

Originally, combineLatest made an array or a dictionary to hold the latest values from each inner stream. It would modify that object if an inner stream changed, then dispatch the result.

Though this minimizes allocations, it also makes the resulting stream more difficult to reason about. Consider this example (taken from Tossable):

const locationOnDown$ = location$._debounce(whenDragIsActive$);
const draggedLocation$ = draggable.value$.addedBy(locationOnDown$);
// …
spring.value$.subscribe(location$);

This looks like it takes the location when the drag started and adds it to the latest emission from draggable. Indeed, when Tossable used a NumericSpring, this is what it did. Because numbers are primitives, each emission from NumericSpring is distinct.

However, Point2DSpring uses combineLatest, which means that older emissions and current emissions all recycle the same object: if location$ is set by a Point2DSpring, locationOnDown$ can change even though whenDragIsActive$ hasn't dispatched. This is non-obvious and difficult to debug.

Therefore, I'm altering the behavior of combineLatest to always dispatch a new object. If this ever introduces a performance problem, we can revert this change (and introduce a _clone operator to replace it). However, I doubt that is worth either the technical complexity or the mental overhead.

Diff Detail

Repository
R13 material-motion/material-motion-js
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
appsforartists created this revision.Oct 4 2017, 8:57 PM
Restricted Application added a reviewer: O2: Material Motion. · View Herald TranscriptOct 4 2017, 8:57 PM
Restricted Application added a project: Material Motion. · View Herald Transcript
Restricted Application added a reviewer: O3: Material JavaScript platform reviewers. · View Herald Transcript
Restricted Application added a reviewer: Material Motion. · View Herald Transcript
Restricted Application completed building Restricted Buildable.Oct 4 2017, 9:00 PM

Adding regression tests

Restricted Application completed building Restricted Buildable.Oct 5 2017, 6:15 PM
featherless accepted this revision.Oct 6 2017, 1:56 PM
This revision is now accepted and ready to land.Oct 6 2017, 1:56 PM
This revision was automatically updated to reflect the committed changes.