Merge rx-recompose into main project #196

acdlite · 2016-06-11T22:08:45Z

No RxJS dependency. Uses observables that conform to Observable spec and implement Symbol.observable.
Configurable interop with stream libraries like RxJS, most, xstream, and Bacon
Renames createComponent to componentFromStream

chicoxyzzy · 2016-06-11T22:14:37Z

src/packages/recompose/__tests__/mapPropsStream-test.js

 const identity = t => t

 test('mapPropsStream maps a stream of owner props to a stream of child props', t => {
  const SmartButton = mapPropsStream(props$ => {
    const { handler: onClick, stream: increment$ } = createEventHandler()
    const count$ = increment$
-      .startWith(0)
-      .scan(total => total + 1)
+      ::startWith(0)


Bind operator is still at stage 0 so syntax may change in future and there are no guarantees it will be included in spec

Well it's a test, so oh well for now :D

The reason I'm importing each helper separately is to ensure as much as possible that we're not depending on any RxJS-specific functionality.

yes, I understand that

chicoxyzzy · 2016-06-11T22:19:33Z

It'll be cool if we can use not only RxJS but also most and other libraries. But this means recompose should use Symbol.observable instead of Observable and I don't sure how to do it proper. Otherwise docs should refer to RxJS 5 specifically IMO.

chicoxyzzy · 2016-06-11T22:30:05Z

also interesting thread
cujojs/most#160

istarkov · 2016-06-11T22:36:41Z

src/packages/recompose/mapPropsStream.js

+const mapPropsStream = transform => BaseComponent => {
+  const factory = createEagerFactory(BaseComponent)
+  return componentFromStream(ownerProps$ =>
+    Observable.create(observer => {


Can't see .create in proposal https://github.com/zenparsing/es-observable
may be better to use new Observable(observer => instead

Good catch!

acdlite · 2016-06-11T22:39:25Z

@chicoxyzzy

It'll be cool if we can use not only RxJS but also most and other libraries

As long as Observable is a global that follows the spec, it should work, right?

chicoxyzzy · 2016-06-11T22:51:40Z

Hmmm.. Probably you are right and it will work. I think I can create PR with some tests for most as well if you are not opposed =)

istarkov · 2016-06-11T23:10:17Z

Good idea at @chicoxyzzy ref cujojs/most#160

It will allow to remove global Observable ref, and the only downside
before using with rxjs without bind syntax is that you will need to convert
Rx.Observable.from(props$).hereICanUseRxJsOperatorsAsIs

chicoxyzzy · 2016-06-11T23:28:43Z

We can still use bind syntax after converting ES-compatible stream to RxJS observable

chicoxyzzy · 2016-06-12T00:03:08Z

Just checked out. RxJS API and Most.js API are not 100% compatible so current docs from PR will work only for Rx but not Most.
We really can remove global Observable ref but in that case we should add [Symbol.observable]() to Recompose streams to make an interop possible. This is good because not Rx nor Most are actually polyfills but libraries for streams (kind of lodash for streams).

chicoxyzzy · 2016-06-12T00:08:17Z

The original Observable from spec just have two methods. But it's possible to easily convert them into Rx or Most like so:

import Rx from 'rxjs/Rx';
import { from } from 'most';

const RxObservable = Rx.Observable.from(SomeObservable);
const MostObservable = from(SomeObservable);

chicoxyzzy · 2016-06-12T00:13:57Z

Eventually this also can make integration with mobx possible (for first time it will be possible using Rx.Observable.from and https://github.com/chicoxyzzy/rx-mobx)

chicoxyzzy · 2016-06-12T00:22:50Z

Redux does the same thing reduxjs/redux#1632

However IMO it's not so trivial to add [Symbol.observable]() method for Rx-Recompose and maybe it's an idea for distinct library

acdlite · 2016-06-12T00:30:22Z

We really can remove global Observable ref but in that case we should add Symbol.observable to Recompose streams to make an interop possible. This is good because not Rx nor Most are actually polyfills but libraries for streams (kind of lodash for streams).

I'm not sure we're on the same page. What is a "Recompose stream"? Are you referring to props$, the stream of props that is provided to mapPropsStream and componentFromStream? My goal is avoid introducing a Recompose-specific stream type entirely by using the observable spec.

The purpose of [Symbol.observable]() is to turn a stream-like value into an Observable. I don't see how it applies here, given that we're using observables directly.

A code example explaining what you mean would be helpful.

acdlite · 2016-06-12T00:37:28Z

Oh wait, I think I see what you mean about removing the global Observable. ~~I don't think it's necessary for most compatibility, but it's a good idea nonetheless.~~ Yeah you're right, it effectively is.

acdlite · 2016-06-12T01:28:23Z

@chicoxyzzy @istarkov Alright, I just pushed a commit that removes the global Observable.

The one downside I discovered to this approach, rather than mandating that your favorite observable library be available as an Observable global, is that every observable will need to be wrapped with Observable.from or something similar. To see what I mean, try deleting these lines from the test file:

recompose/src/packages/recompose/__tests__/mapPropsStream-test.js

Lines 20 to 29 in fbb62b6

    
           const mapPropsStream = transform => 
        
             _mapPropsStream(props$ => transform(Observable.from(props$))) 
        
           const createEventHandler = () => { 
        
             const { stream, handler } = _createEventHandler() 
        
             return { 
        
               handler, 
        
               stream: Observable.from(stream) 
        
             } 
        
           }

.

I'm thinking maybe we could expose a global module like configureObservable that allows you to specify how to convert observables to the desired form:

import configureObservable from 'recompose/configureObservable'
configureObservable(observable => Rx.Observable.from(observable))

// Then...
mapPropsStream(props$ => {
  // props$ is an RxJS stream
})

What do you think of that idea?

acdlite · 2016-06-12T01:53:29Z

Pushed an implementation of configureObservable. Seems a little gross, but gets the job done.

typeetfunc · 2016-06-12T09:05:53Z

src/packages/recompose/createEventHandler.js

+
+const createEventHandler = () => {
+  const emitter = createChangeEmitter()
+  const transform = getTransform()


@acdlite Maybe allow pass to configureObservable subject-like anything? And use change-emitter if only passed Observable dont has method next?

@typeetfunc Subject is Rx-related thing. There are no subjects in most. There is https://github.com/TylorS/most-subject but @TylorS once said in most.js gitter chat that it's more like motorcycle/cycle-related thing.
Discussion about Subjects in most cujojs/most#164

@chicoxyzzy Thanks for links. Also observer dont has interop point like symbol-observable - tc39/proposal-observable#89. Perhaps my proposal was a bit hasty :)

chicoxyzzy · 2016-06-12T11:16:44Z

@acdlite I think it's worth it 👍

chicoxyzzy · 2016-06-12T11:38:01Z

One thing I can suggest is to add tests for most to be sure Observable iterop compatibility works fine

istarkov · 2016-06-12T14:21:29Z

About 2 commits above:

Import fix is because of import { Observable } from 'rxjs' will import all operators.
Nobind test as a lot of people prefer rxjs operators without bind, and test is the best documentation.

chicoxyzzy · 2016-06-12T16:01:33Z

Just trying to implement some most tests (actually an analogue of first one from mapStreamProps) but it seems like SmartButton doesn't get mounted by enzyme for some reason 😕

miketembo · 2016-06-12T16:56:05Z

help me understand

acdlite · 2016-06-12T18:42:56Z

@istarkov Please don't add commits to a PR that's not yours without approval. Best way to do that is to submit a PR to this branch :)

What is the value of the test file without bind syntax? I don't see any.

typeetfunc · 2016-06-13T11:54:03Z

@acdlite What about context? Maybe pass them second arg in componentFromStream? Context doesnt require subscription, becuase its constanst usually. Its maybe useful for pass global streams like as actions or state streams.

acdlite · 2016-06-13T14:53:57Z

@typeetfunc

const enhance = compose(
  getContext(contextTypes),
  mapPropsStream(props$ => {
    // context is part of props$
  })
)

typeetfunc · 2016-06-13T16:27:36Z

@acdlite oh sorry, i missed it. Thanks :)

istarkov · 2016-06-13T16:40:42Z

src/packages/recompose/componentFromStream.js

-      this.subscription = this.vdom$.subscribe(
-        vdom => {
+      this.subscription = this.vdom$.subscribe({
+        next: vdom => {
          this.didReceiveVdom = true
          if (!this.componentHasMounted) {
            this.state = { vdom }


Why check on hasMounted? (It was a bug in react, but it solved)
Also looks like this.state = {} could be changed on this.setState as this method does not called from contructor

It was a bug in react, but it solved

Link?

Also looks like this.state = {} could be changed on this.setState as this method does not called from contructor

You're right. This is left over from when it used to subscribe inside the constructor.

Link ?

One of:
facebook/react#5719
fixed in 15.1

The check is to prevent calling setState if the component has already unmounted. That issue seems unrelated? Could be wrong.

acdlite · 2016-06-13T17:07:07Z

Re-commenting down here because original comment is on outdated diff:

@istarkov The componentDidUnmount check is to prevent calling setState if the component has already unmounted. The issue you linked to seems unrelated? Could be wrong.

istarkov · 2016-06-13T17:12:18Z

this.subscription.unsubscribe() should prevent any calls there, so I deceided that it is because of some bugs. But now I see that in bug case this.subscription.unsubscribe() should throw.

Looks like there is no need to check this.componentDidUnmount

acdlite · 2016-06-13T17:19:49Z

Oh duh that's right

chicoxyzzy · 2016-06-14T21:28:10Z

Didn't made any progress with my most.js tests yet and sorry for sort of offtopic but I think you may be interested in future of function bind syntax tc39/proposal-bind-operator#24 (comment)

TL;DR

it will not be ::
probably proposal will be splitted into separate "method extraction" and "functional pipelining" proposals
it will not be added any soon

istarkov · 2016-06-14T23:51:00Z

src/packages/recompose/componentFromStream.js


    // Stream of vdom
-    vdom$ = propsToVdom(this.props$);
+    vdom$ = toObservable(propsToVdom(this.props$));

    didReceiveVdom = false;


vdom: null could me moved into state initializer, so there will be no need in didReceiveVdom at all

chicoxyzzy · 2016-06-15T00:16:31Z

Summing up my previous comment I propose to add test which @istarkov provided earlier (without bind syntax) so no one will suffer refactoring their code after getting inspiration from test files. Also IMO tests should be written in JavaScript, not some dialect no one will understand one day 😃

acdlite · 2016-06-15T17:10:21Z

Yeah I'll remove the :: operator. I wasn't too concerned about it because it's recommended by the RxJS docs, but I didn't consider that people might read the tests and get confused.

The tests in mapPropsStream need to be rewritten anyway, I think. Not too happy with them right now.

acdlite · 2016-06-16T03:15:25Z

@istarkov @chicoxyzzy Just added tests for compatibility with RxJS 4/5, most, xstream, and Bacon. I've also added modules that export the config for each library. Are there any other stream libraries that we should include?

chicoxyzzy · 2016-06-16T03:16:18Z

Wow! Last commit is super cool!

chicoxyzzy · 2016-06-16T03:21:58Z

@acdlite the only library I know besides these is Kefir. It's API should be similiar to Bacon

acdlite · 2016-06-16T03:31:05Z

@chicoxyzzy Huzzah, and it already does all the hard work for us! https://rpominov.github.io/kefir/#from-es-observable

acdlite · 2016-06-16T03:34:06Z

I actually Kefir's naming better, too: fromESObservable and toESObservable, rather than just fromObservable and toObservable. Much clearer. I'll change it.

acdlite added 2 commits June 11, 2016 15:07

Merge rx-recompose into main project

50d86f5

createComponent -> componentFromStream

61eb5fd

chicoxyzzy reviewed Jun 11, 2016
View reviewed changes

istarkov reviewed Jun 11, 2016
View reviewed changes

Use Observable constructor instead of Observable.create

534415d

Remove global observable and use Symbol.observable instead

fbb62b6

configureObservable applies a pre-transform to all observables

d9bb6f7

Empty, don't need it

12de7e3

typeetfunc reviewed Jun 12, 2016
View reviewed changes

Fix rxjs import

5732a70

acdlite force-pushed the merge-rx-recompose branch from d278dd8 to 5732a70 Compare June 13, 2016 16:11

Configure transforms to and from observables

c0ccf69

istarkov reviewed Jun 13, 2016
View reviewed changes

acdlite added 3 commits June 13, 2016 09:54

Docs for configureObservable

b3c5d4d

Typo

95fe144

No need to check if component has mounted

14ae089

acdlite added 2 commits June 14, 2016 09:15

No need to check if component has unmounted

f5f1cd5

Remove Observable as global from eslintrc

e3ff58b

istarkov reviewed Jun 14, 2016
View reviewed changes

acdlite added 2 commits June 15, 2016 11:07

Don't need didReceiveProps check

ea8ec6d

Add configs for popular stream libraries

1dfa6f3

acdlite added 3 commits June 15, 2016 20:42

Add config for Kefir

af51c21

Implement Symbol.observable, for correctness

01f0aab

Bump rxjs to latest beta

b47d748

acdlite merged commit c32e089 into master Jun 16, 2016

istarkov deleted the merge-rx-recompose branch April 16, 2017 16:15

Merge rx-recompose into main project #196

Merge rx-recompose into main project #196

Conversation

acdlite commented Jun 11, 2016 • edited 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

chicoxyzzy commented Jun 11, 2016 • edited Loading

chicoxyzzy commented Jun 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Jun 11, 2016

chicoxyzzy commented Jun 11, 2016

istarkov commented Jun 11, 2016 • edited Loading

chicoxyzzy commented Jun 11, 2016 • edited Loading

chicoxyzzy commented Jun 12, 2016

chicoxyzzy commented Jun 12, 2016 • edited Loading

chicoxyzzy commented Jun 12, 2016 • edited Loading

chicoxyzzy commented Jun 12, 2016

acdlite commented Jun 12, 2016 • edited Loading

acdlite commented Jun 12, 2016 • edited Loading

acdlite commented Jun 12, 2016 • edited Loading

acdlite commented Jun 12, 2016

Choose a reason for hiding this comment

chicoxyzzy Jun 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chicoxyzzy commented Jun 12, 2016

chicoxyzzy commented Jun 12, 2016 • edited Loading

istarkov commented Jun 12, 2016

chicoxyzzy commented Jun 12, 2016

miketembo commented Jun 12, 2016

acdlite commented Jun 12, 2016

typeetfunc commented Jun 13, 2016

acdlite commented Jun 13, 2016

typeetfunc commented Jun 13, 2016

istarkov Jun 13, 2016 • edited 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

acdlite commented Jun 13, 2016

istarkov commented Jun 13, 2016

acdlite commented Jun 13, 2016

chicoxyzzy commented Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

chicoxyzzy commented Jun 15, 2016 • edited Loading

acdlite commented Jun 15, 2016

acdlite commented Jun 16, 2016

chicoxyzzy commented Jun 16, 2016

chicoxyzzy commented Jun 16, 2016 • edited Loading

acdlite commented Jun 16, 2016

acdlite commented Jun 16, 2016

acdlite commented Jun 11, 2016 •

edited

Loading

chicoxyzzy commented Jun 11, 2016 •

edited

Loading

istarkov commented Jun 11, 2016 •

edited

Loading

chicoxyzzy commented Jun 11, 2016 •

edited

Loading

chicoxyzzy commented Jun 12, 2016 •

edited

Loading

chicoxyzzy commented Jun 12, 2016 •

edited

Loading

acdlite commented Jun 12, 2016 •

edited

Loading

acdlite commented Jun 12, 2016 •

edited

Loading

acdlite commented Jun 12, 2016 •

edited

Loading

chicoxyzzy Jun 12, 2016 •

edited

Loading

chicoxyzzy commented Jun 12, 2016 •

edited

Loading

istarkov Jun 13, 2016 •

edited

Loading

chicoxyzzy commented Jun 14, 2016 •

edited

Loading

chicoxyzzy commented Jun 15, 2016 •

edited

Loading

chicoxyzzy commented Jun 16, 2016 •

edited

Loading