Make consistent API decision about named properties

[appsforartists]

e.g. should we use the form

createProperty(342);

or

createProperty({ withInitialValue: 342 });

I was originally inclined to use the named arguments pattern (and have done that in many places throughout the lib); however, it looks a bit awkward when there's only one argument, especially when operators written in other libraries don't use that pattern.

Should we prefer motionObservable.pluck('x') or motionObservable.pluck({ path: 'x' })? The former is what Rx and xstream use. We originally preferred the latter, but since we started implementing our own Observable library, it's been a mix. We need to pick one strategy and use it as consistently as we can.

[appsforartists]

I took all the current operators and looked at what they might look like if we preferred named params vs. positional ones.

For most of our operators, either would be fine. For the mathematical ones (addedTo, subtractedBy), ones with constants (startWith, rewriteTo), ones with lists (merge, isAnyOf), and common foundational ones (_filter, _map) the named pattern is detrimental. If we stay with the positional pattern, some operators (rewrite, _reactiveNextOperator) will need to accept config objects as the last parameter, which means we shouldn't use rest parameters in our APIs.

I want to think about this some more/get input from other developers before making a decision here. For now, I'm going to err towards positional arguments and passing a dictionary as the final value when needed.

Named params

(names that don't aid understanding are commented out)

stream
  // .appendUnit({unit: 'px'})
  .dedupe({areEqual: deepEqual})
  // .delayBy({time: 100})
  // .distanceFrom({origin: {x: 0, y: 0}})
  // .ignoreUntil({expectedValue: 100})
  .inverted()
  // .isAnyOf({matches: [1, 2, 3]})
  .log({label: 'current location', path: 'x'})
  .clamp({lowerBound: 0, upperBound: 100})
  // .merge({others: [a$, b$]})
  // .dividedBy({denominator: 100})
  // .addedTo({amount: {x: 0, y: 100}})
  // .pluck({path: 'y'})
  .rewrite({
    map: {[NavState.CLOSED]: true, [NavState.OPEN]: false},
    dispatchOnKeyChange: false
  })
  .rewriteRange({fromStart: 0, fromEnd: 1, toStart: 50, toEnd: 100})
  // .rewriteTo({value: 100})
  // .multipliedBy({coefficient: 20})
  .slidingWindow({size: 5})
  // .startWith({value: 1})
  // .threshold({limit: 50})
  .thresholdRange({start: 20, end: 30})
  .timestamp()
  .velocity({pulse: dragAtRest$})
  ._debounce({pulse: frame$})
  // ._filter({predicate: isNaN})
  ._flattenIterables()
  // ._map({transform: (value) => value ** 2})
  ._multicast()
  // ._nextOperator({operation({value, nextChannel}) => nextChannel(value)})
  // ._reactiveMap({transform: (a, b) => a * b, args: [a$, b$]})
  ._reactiveNextOperator(
      {operation({value, nextChannel}) => nextChannel(value), args: [a$, b$], waitForAllValues: true})
  ._read()
  ._remember()
  // ._tap({transform: (value) => sideEffect(value)})

Named params

(Names that are make the API less usable are commented)

stream
  .appendUnit({unit: 'px'})
  .dedupe({areEqual: deepEqual})
  .delayBy({time: 100})
  .distanceFrom({origin: {x: 0, y: 0}})
  .ignoreUntil({expectedValue: 100})
  .inverted()
  // .isAnyOf({matches: [1, 2, 3]})
  .log({label: 'current location', path: 'x'})
  .clamp({lowerBound: 0, upperBound: 100})
  // .merge({others: [a$, b$]})
  // .dividedBy({denominator: 100})
  // .addedTo({amount: {x: 0, y: 100}})
  .pluck({path: 'y'})
  .rewrite({
    map: {[NavState.CLOSED]: true, [NavState.OPEN]: false},
    dispatchOnKeyChange: false
  })
  .rewriteRange({fromStart: 0, fromEnd: 1, toStart: 50, toEnd: 100})
  // .rewriteTo({value: 100})
  .multipliedBy({coefficient: 20})
  .slidingWindow({size: 5})
  // .startWith({value: 1})
  .threshold({limit: 50})
  .thresholdRange({start: 20, end: 30})
  .timestamp()
  .velocity({pulse: dragAtRest$})
  ._debounce({pulse: frame$})
  // ._filter({predicate: isNaN})
  ._flattenIterables()
  // ._map({transform: (value) => value ** 2})
  ._multicast()
  ._nextOperator({operation({value, nextChannel}) => nextChannel(value)})
  ._reactiveMap({transform: (a, b) => a * b, args: [a$, b$]})
  ._reactiveNextOperator(
      {operation({value, nextChannel}) => nextChannel(value), args: [a$, b$], waitForAllValues: true})
  ._read()
  ._remember()
  ._tap({transform: (value) => sideEffect(value)})

Positional args

(arguments that would be easier to read with names are commented)

stream
  .appendUnit('px')
  // .dedupe(deepEqual)
  .delayBy(100)
  .distanceFrom({x: 0, y: 0})
  .ignoreUntil(100)
  .inverted()
  .isAnyOf([1, 2, 3])
  .log('current location', 'x')
  .lowerBound(0)
  .upperBound(100)
  .merge([a$, b$])
  .dividedBy(100)
  .addedTo({x: 0, y: 100})
  .pluck('y')
  // .rewrite(
      {[NavState.CLOSED]: true, [NavState.OPEN]: false},
      {dispatchOnKeyChange: false})
  .rewriteRange({fromStart: 0, fromEnd: 1, toStart: 50, toEnd: 100})
  .rewriteTo(100)
  .multipliedBy(20)
  // .slidingWindow(5)
  .startWith(1)
  .threshold(50)
  .thresholdRange(20, 30)
  .timestamp()
  // .velocity(dragAtRest$)
  // ._debounce(frame$)
  ._filter(isNaN)
  ._flattenIterables()
  ._map((value) => value ** 2)
  ._multicast()
  ._nextOperator((value, nextChannel) => nextChannel(value))
  ._reactiveMap((a, b) => a * b, [a$, b$])
  // ._reactiveNextOperator((value, nextChannel) => nextChannel(value), [a$, b$], { waitForAllValues: true })
  ._read()
  ._remember()
  ._tap((value) => sideEffect(value))

Positional args

(operators that would need a dictionary anyway are commented)

stream
  .appendUnit('px')
  .dedupe(deepEqual)
  .delayBy(100)
  .distanceFrom({x: 0, y: 0})
  .ignoreUntil(100)
  .inverted()
  .isAnyOf([1, 2, 3])
  .log('current location', 'x')
  .lowerBound(0)
  .upperBound(100)
  .merge([a$, b$])
  .dividedBy(100)
  .addedTo({x: 0, y: 100})
  .pluck('y')
  // .rewrite(
      {[NavState.CLOSED]: true, [NavState.OPEN]: false},
      {dispatchOnKeyChange: false})
  // .rewriteRange({fromStart: 0, fromEnd: 1, toStart: 50, toEnd: 100})
  .rewriteTo(100)
  .multipliedBy(20)
  .slidingWindow(5)
  .startWith(1)
  .threshold(50)
  .thresholdRange(20, 30)
  .timestamp()
  .velocity(dragAtRest$)
  ._debounce(frame$)
  ._filter(isNaN)
  ._flattenIterables()
  ._map((value) => value ** 2)
  ._multicast()
  ._nextOperator((value, nextChannel) => nextChannel(value))
  ._reactiveMap((a, b) => a * b, [a$, b$])
  // ._reactiveNextOperator((value, nextChannel) => nextChannel(value), [a$, b$], { waitForAllValues: true })
  ._read()
  ._remember()
  ._tap((value) => sideEffect(value))

[appsforartists]

If we go with arrays, we should be consistent there too (e.g. change the signature of merge).

[appsforartists]

Glancing through these again, and seeing how many operators I've added ReactiveMappableOptions too, I'm tempted to go back to named args.

Need to get the types right first, so I can make sure the named args implementation is as DRY as possible.

[appsforartists]

This makes sense for operators and interactions. I'll hold off on changing aggregators - seems like it would only make them less readable.

[appsforartists]

There are some operators where named args make the API worse: pluck, isAnyOf, added/subtracted/multiplied/dividedBy, distanceFrom, etc. I wonder if I should add a positional signature for those too. Every operator would accept named args, but a few would also accept a positional alternative.

[appsforartists]

🎊 My diff is ready for review at http://codereview.cc/D3419