ViacomInc / data-point

JavaScript Utility for collecting, processing and transforming data
Apache License 2.0
67 stars 34 forks source link

🏁 Change ReducerFunctions to receive (value, ctx) #117

Closed raingerber closed 6 years ago

raingerber commented 6 years ago

Problem description:

When executing reducers, the current value is stored on the accumulator, but that can be somewhat awkward in function reducers:

// in this lambda, we need to pull the value off the accumulator before using it
const reducer = (accumulator) => accumulator.value + 1

Suggested solution:

Do not store the value on the accumulator. Instead, have two parameters: value and context. The value is equivalent to the current accumulator.value, and context will contain the other properties that are stored in the accumulator (such as values, params, etc). This would simplify our function reducers:

// the example from above is simpler now
const reducer = (value) => value + 1

// and we still have the context if we need it
const reducer = (value, context) => value + context.params.otherValue

This would also make it easier to use third-party functions as DataPoint reducers. For example, say we're trying to use lodash's flatten as a reducer:

// currently we'd have to do this:
const flattenReducer = (accumulator) => _.flatten(accumulator.value)

dataPoint.transform(['request:r', flattenReducer], inputData)

// but if value is not stored on the accumulator, we can use _.flatten directly!

dataPoint.transform(['request:r', _.flatten], inputData)
acatl commented 6 years ago

bump head

This has been the worst API decision I've made in this library so far...


I love this change, but to make it we must provide a codemod that will help others refactor.

acatl commented 6 years ago

FYI @jsaterfiel

acatl commented 6 years ago

The accumulator should still exist as it is on all of DataPoint internals, this feature should only change for how the ReducerFunctions work, meaning only the API expected of the reducer functions should change, nothing more.

Only this part of the code base should change + any tests

https://github.com/ViacomInc/data-point/blob/a1c64769c57c0b20fb7e4f132445bfd26021d1d2/packages/data-point/lib/reducer-function/resolve.js#L12-L33

IMPORTANT: Please hold off on working on this feature until we have the following tasks done and merged: