Melody streams #102

krakenfuss · 2019-02-22T14:15:51Z

What changed in this PR:

Adding new core package melody-streams.
This package aims to reduce the API surface and the amount of technologies an engineer has to know to be productive with Melody to a single one: Rx.js
It is based on the simplicity concept of hooks, while it works without any hooks and abstracts data handling and component updating completely to streams.

malte-wessel · 2019-02-23T11:32:00Z

packages/melody-streams/__tests__/mergeObjectSpec.js

+            biz: 'baz',
+            arr: [1, 2, 3, 4],
+        };
+        testWith(mergeObject(spec), () => {}, []);


Since the outcomes of these test are quite small, could we inline the expect(...).toEqual(...) instead of using expect and toMatchSnapshot inside testWith?

This would make reading the tests way easier.

malte-wessel · 2019-02-23T11:41:12Z

packages/melody-streams/src/index.js

+ */
+
+export { createComponent } from './component';
+export { createState } from './operators/createState';


What do you think about separating the functions in ./operators into helpers and operators?

Actual operators are:

combine: Observable mergeIntoObjects: Observable mergeObject: Observable

Helpers have a different API:

withElement: [Element => Subscription, Subject] withEvent: [Element => Subscription, Subject]; createState: [Subject, state => void, () => state];

And maybe use withEvent instead of attachEvent for consistency?

I also like the name withState

malte-wessel · 2019-02-23T11:43:13Z

packages/melody-streams/src/component.js

+    // part of the public API
+    this.el = element;
+    this.refs = {};
+    this.props = new BehaviorSubject({});


Not sure, but this.props should be a plain object, otherwise this could break the dev tools.
Maybe we should name it this.propsStream and set this.props inside apply as soon as it receives new props

Good point. Changed it.

malte-wessel · 2019-02-23T11:44:54Z

packages/melody-streams/src/component.js

+        this.props.next(props);
+        if (this.subscriptions.length === 0) {
+            const t = this.getTransform({
+                dispatch(eventName, detail, options = {}) {


dispatchEvent or dispatchToDom or dispatchDomEvent ?

@pago suggested dispatchCustomEvent, since highlights that we’re not dispatching standard DOM events like 'click'. What do you think?

packages/melody-streams/src/util/shallowEqual.js

malte-wessel · 2019-02-23T11:51:29Z

packages/melody-streams/src/operators/withElement.js

+
+import { Subject } from 'rxjs';
+
+export const withElement = handler => {


The name handler seems a bit confusing to me, I would expect that to be a callback function. Maybe createObservable or something would make it clearer.

malte-wessel · 2019-02-23T11:52:34Z

packages/melody-streams/__tests__/withElementSpec.js

+        const exec = refHandler();
+        testWith(subj, next(exec), ['foo', 'bar']);
+    });
+});


what if handler is not a function

what if handler doesn't return an observable

malte-wessel · 2019-02-23T11:53:51Z

packages/melody-streams/__tests__/util/createTestComponent.js

@@ -0,0 +1,65 @@
+/**


Can be removed I guess

Can this be removed?

malte-wessel · 2019-02-23T12:04:38Z

packages/melody-streams/__tests__/componentSpec.js

+            return props;
+        }, template);
+        // Snapshot should have 2 entries
+        testWithArguments(subj, render, [


We should also check the actual DOM outcome with root.outerHtml

malte-wessel · 2019-02-23T12:10:23Z

packages/melody-streams/src/component.js

+            });
+            const s = t
+                .pipe(distinctUntilChanged(shallowEqual))
+                .subscribe(state => {


what happens when an error occurs?

what happens when the stream completes early?

would it make sense to check for a valid state shape (plain object with only primitives as values) here?

malte-wessel · 2019-02-23T12:10:58Z

packages/melody-streams/src/component.js

+                },
+                props: this.props,
+                updates: this.updates,
+                subscribe: obs => this.subscriptions.push(obs.subscribe()),


What if obs is not an observable?

Maybe we should provide a custom error here, something like Error: subscription object must be an Observable.

We can probably do that later if we really want to do it. If obs doesn't have a subscribe method. Or we "just" provide a .d.ts file to automate that.

I think we should throw an error if obs is not an Observable. This will make it easier for engineers, that are not familiar with the API. Otherwise you would get something like subscribe is not a function and that would be confusing, at least to me.

pago · 2019-02-25T05:35:59Z

packages/melody-streams/src/index.js

+
+export { createComponent } from './component';
+export { createState } from './operators/createState';
+export { mergeObject } from './operators/mergeObject';


I'd argue that mergeObject and mergeIntoObject should not be part of the public API. combine can do both their jobs and its easier to learn just one function instead of 3.

pago · 2019-02-25T10:09:12Z

Let's also address this issue directly: #76
i.e. let's add a render method so that we won't have to teach melody-component just to render a component.

danielcaldas · 2019-02-22T15:30:56Z

packages/melody-streams/src/operators/createState.js

+export const createState = initialValue => {
+    const subj = new BehaviorSubject(initialValue);
+
+    const mutator = newValue => {


Do we really want to allow the mutator argument to be a function? If the same work newValue(subj.getValue()) can be done on the client side I think it's a bit redundant to add this option. Passing only the raw value on the other hand would simplify the implementation and create a more uniform API for the mutator.

I like the extra affordance of this particular overload and believe it could enable some very nice patterns (though I'm not sure yet which those would be). Adding it later would not be an option. Removing it also won't be possible.

Would you object strongly against keeping it? Or is it rather a preference?

If you ask me, this is a quite useful feature, e.g. for implementing a state reducer.

I also believe this is a super useful thing. Having an option to pass in a function makes the API quite flexible and super powerful. I would strongly advise to keep it.

danielcaldas · 2019-02-22T15:43:31Z

packages/melody-streams/src/operators/createState.js

+import { BehaviorSubject } from 'rxjs';
+
+export const createState = initialValue => {
+    const subj = new BehaviorSubject(initialValue);


Just an idea maybe for future development. We could make use of ReplaySubject and have a different API than createState, let's say it would be called createTimeTravelerState that would allow us to easily to expose something like a .rewind() method on the component to restore previous states. Might be a little farfetched tough 😄

danielcaldas · 2019-02-25T09:51:41Z

packages/melody-streams/src/component.js

+        this.props.next(props);
+        if (this.subscriptions.length === 0) {
+            const t = this.getTransform({
+                dispatch(eventName, detail, options = {}) {


@pago suggested dispatchCustomEvent, since highlights that we’re not dispatching standard DOM events like 'click'. What do you think?

danielcaldas · 2019-02-25T10:03:01Z

packages/melody-streams/src/component.js

+                },
+                props: this.props,
+                updates: this.updates,
+                subscribe: obs => this.subscriptions.push(obs.subscribe()),


Maybe we should provide a custom error here, something like Error: subscription object must be an Observable.

danielcaldas · 2019-02-25T11:12:45Z

packages/melody-streams/src/component.js

+};
+
+export const baseCreateComponent = (transform, templateFnOrObj) => {
+    const template = templateFnOrObj.render


Should we check for type here? typeof templateFnOrObj.render === 'function'

No. If it's not a function, it'll throw. The only way to get into this code path is if you're not using twig templates but instead code the templates in JavaScript directly. It's a legacy thing.

danielcaldas · 2019-02-25T11:14:02Z

packages/melody-streams/src/component.js

+    if (args.length >= baseCreateComponent.length) {
+        return baseCreateComponent.apply(null, args);
+    }
+    return (...args2) => createComponent.apply(null, args.concat(args2));


Rename suggestion: args2 -> _args

Or use lodash and its curry function directly. _args is not much better though. I'd go for args => preloadedArgs and args2 => nextArgs or something like that if it was a concern. But I don't think this needs to be done now.

ayusharma · 2019-02-25T13:13:59Z

packages/melody-streams/__tests__/componentSpec.js

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { assert } from 'chai';


There is already an issue (#88) to remove chai's matchers with Jest's matchers.

pago

Still open and needs to be done before merge:

Export a render method (can be copy-pasted from melody-component)
Support returning object from the transform
Potentially remove packages/melody-streams/__tests__/util/createTestComponent.js if not needed
Do we keep the setState(oldState => newState) overload for createState?

pago · 2019-02-25T09:25:26Z

packages/melody-streams/__tests__/createStateSpec.js

+describe('createState', () => {
+    it('should create an Observable from initial data', () => {
+        const [count] = createState(0);
+        count.subscribe(x => {


Maybe more reliable with a test spy

pago · 2019-03-01T06:18:31Z

packages/melody-streams/__tests__/util/createTestComponent.js

@@ -0,0 +1,65 @@
+/**


Can this be removed?

pago · 2019-03-01T06:28:19Z

packages/melody-streams/src/component.js

+                },
+                props: this.props,
+                updates: this.updates,
+                subscribe: obs => this.subscriptions.push(obs.subscribe()),


We can probably do that later if we really want to do it. If obs doesn't have a subscribe method. Or we "just" provide a .d.ts file to automate that.

pago · 2019-03-01T06:29:55Z

packages/melody-streams/src/component.js

+};
+
+export const baseCreateComponent = (transform, templateFnOrObj) => {
+    const template = templateFnOrObj.render


No. If it's not a function, it'll throw. The only way to get into this code path is if you're not using twig templates but instead code the templates in JavaScript directly. It's a legacy thing.

pago · 2019-03-01T06:31:29Z

packages/melody-streams/src/component.js

+    if (args.length >= baseCreateComponent.length) {
+        return baseCreateComponent.apply(null, args);
+    }
+    return (...args2) => createComponent.apply(null, args.concat(args2));


Or use lodash and its curry function directly. _args is not much better though. I'd go for args => preloadedArgs and args2 => nextArgs or something like that if it was a concern. But I don't think this needs to be done now.

pago · 2019-03-01T06:36:41Z

packages/melody-streams/src/operators/createState.js

+export const createState = initialValue => {
+    const subj = new BehaviorSubject(initialValue);
+
+    const mutator = newValue => {


I like the extra affordance of this particular overload and believe it could enable some very nice patterns (though I'm not sure yet which those would be). Adding it later would not be an option. Removing it also won't be possible.

Would you object strongly against keeping it? Or is it rather a preference?

pago · 2019-03-01T06:37:53Z

packages/melody-streams/src/operators/withElement.js

+import { BehaviorSubject } from 'rxjs';
+
+export const withElement = (handler, initialValue) => {
+    const subj = initialValue


I think we need to check for undefined explicitly. Otherwise withElement(fn, false) and similar cases won't work.

pago · 2019-03-01T06:41:52Z

packages/melody-streams/src/component.js

+    ChildComponent.prototype.render = function() {
+        return template(this.state);
+    };
+    ChildComponent.prototype.getTransform = transform;


If we want to support cases where transform returns an object of streams we'll need to ensure that the return value is always wrapped in combine.

Suggested change

ChildComponent.prototype.getTransform = transform;

ChildComponent.prototype.getTransform = api => combine(transform(api));

If the transform returns an observable, combine will simply return that. Otherwise it'll do its magic on the returned object.

pago · 2019-03-01T06:43:29Z

packages/melody-streams/src/index.js

+ * limitations under the License.
+ */
+
+export { createComponent } from './component';


We'll still need to expose a render function to avoid having to also depend on melody-component.

… this package.

krakenfuss added 4 commits February 21, 2019 13:18

Add new package melody-streams

38a41b4

Add tests for attachEvent, withElement, createState

1e25f8c

Operator tests done

b90b6c9

First component api tests

897abc6

malte-wessel reviewed Feb 23, 2019

View reviewed changes

pago reviewed Feb 25, 2019

View reviewed changes

danielcaldas reviewed Feb 25, 2019

View reviewed changes

ayusharma reviewed Feb 25, 2019

View reviewed changes

krakenfuss added 5 commits February 27, 2019 21:44

Refactoring after comments. Adding more tests on component spec.

f9b94ab

Add combineRefs

4c849ea

Add melody-streams to changelog

52aaa3d

More detailed tests on combineRefs

359c2b3

Fix combineRefs spec description

4ec144b

pago suggested changes Mar 1, 2019

View reviewed changes

Refactoring after code review. Adding render from melody-component to…

173cb14

… this package.

krakenfuss requested a review from pago March 5, 2019 07:50

pago merged commit 06c7e8c into trivago:master Mar 6, 2019


		import { Subject } from 'rxjs';

		export const withElement = handler => {

	ChildComponent.prototype.getTransform = transform;
	ChildComponent.prototype.getTransform = api => combine(transform(api));

Melody streams #102

Melody streams #102

Conversation

krakenfuss commented Feb 22, 2019

What changed in this PR:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pago commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pago left a comment • edited by krakenfuss Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pago left a comment •

edited by krakenfuss

Loading