[fixed] rewrite waiting for an emission from every stream before working
ClosedPublic

Authored by appsforartists on Sep 6 2017, 8:28 PM.

Details

Summary

Formerly, switchMe$.rewrite({ a: a$, b: b$ }) wouldn't dispatch anything until both a$ and b$ had dispatched, event if switchMe$ had only dispatched a. This resolves that issue.

The solution was to add an option to _reactiveNextOperator and combineLatest to determine whether or not they should wait until all children have dispatched before themselves dispatching.

In order to add an option to _reactiveNextOperator, its signature needed to change. Instead of accepting its arguments as positional arguments, it now accepts them in a single list. This is consistent with our other APIs like isAnyOf. To support strong typing, it still passes values into its operation positionally. ... must come at the end of a function signature, but not a function call. Thus, this shouldn't cause any issues.

Normally for a change of this magnitude, I'd write a codemod, but since I don't believe anyone but me has written code against these APIs, I've made the changes by hand in my own projects.

There's a bigger API design question (should we use named args everywhere, or pass options dictionaries to APIs like _reactiveNextOperator?) captured in https://github.com/material-motion/material-motion-js/issues/193#issuecomment-327636972

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.Sep 6 2017, 8:28 PM
Restricted Application added a reviewer: O2: Material Motion. · View Herald TranscriptSep 6 2017, 8:28 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 failed to build Restricted Buildable!Sep 6 2017, 8:31 PM
Restricted Application completed building Restricted Buildable.Sep 6 2017, 8:41 PM
featherless added inline comments.
packages/core/src/operators/scaledBy.ts
37

Why must these all be arrays now?

appsforartists added inline comments.Sep 12 2017, 6:01 PM
packages/core/src/operators/scaledBy.ts
37

TypeScript doesn't let you have a variable number of arguments before a known argument. This is not a valid signature:

function reactiveMap(transform, ...streams, options)

Removing the ... makes streams into an Array, so I can write this signature:

function reactiveMap(transform, streams, options)

So, the short answer is that I needed to make streams a fixed length so I could add options as the final parameter. There's a bigger discussion to be had as to whether options should be the last parameter, or if every operator should take a single dictionary argument (which could contain individual options). That discussion is in https://github.com/material-motion/material-motion-js/issues/193#issuecomment-327636972.

featherless accepted this revision.Sep 20 2017, 5:30 PM

I can't adequately review this change because it sounds like a Typescript "best practices" sort of change. I'll lgtm to unblock you but encourage you to find a typescript reviewer to weigh in on this.

This revision is now accepted and ready to land.Sep 20 2017, 5:30 PM
This revision was automatically updated to reflect the committed changes.