Use Rx5 by default #9

jseminck · 2017-09-03T10:03:54Z

Started working on porting the library to use Rx5 by default.

Updated the getAdapter() and test setup
Changed peerDependency from rx to rxjs
Updated README
Updated blog example: changed to import local rxConnect instead of the version currently on NPM, and refactored the code to use Rx5 syntax
Updated wikistream example: changed to import local rxConnect and removed setting up the adapter

What's left to do:

Wikistream example is broken (as discussed). It seems wikipedia changed the API
Update the docs + codepen examples. I don't think I can do this because they refer to the version hosted on NPM which is still the latest version that uses Rx4 by default. Once the new version is released, I can change these.
I have a small wish to replace Codepen by Codesandbox ( https://codesandbox.io ) - it allows specifying the dependencies and pulls them straight from NPM, so it's easier to see what dependencies a project has.

…n't being shown

jseminck · 2017-09-03T10:06:08Z

BTW I am not sure if the adapter approach is really good. We found out that we were including both Rx4 and Rx5 in our bundle because currently rx-connect has a peerDependency on Rx4, even though we wanted to use Rx5.

It turns out we had Rx4 in our node_modules directory because it was a dependency of one of our devDependencies (chimp) and webpack decided to bundle it anyway...

Anyway, this PR will fix that for us. But it might still impact users that want to use the Rx4 version... IMHO it's better just to advice them to use the older version of rx-connect instead (until some new features are added, if ever).

bsideup · 2017-09-04T05:18:01Z

Hi @jseminck,

Great job! And thanks for pointing me at CodeSandbox, it looks great indeed :)

I'll move code examples from docs to the repo and point CodeSandbox at them, most probably today, so that we can fix the docs together with this PR, ok?

bsideup · 2017-09-04T19:48:24Z

@jseminck done :) Now you can update the docs with RxJS5, see https://github.com/bsideup/rx-connect/tree/master/examples/docs for sources (FYI you can run locally with yarn serve)

bsideup · 2017-09-05T07:32:44Z

README.md

@@ -53,7 +53,7 @@ with this:
 import { rxConnect } from "rx-connect";

 @rxConnect(
-    Rx.Observable.timer(0, 1000).timestamp()


I'm quite sure it's still there :)
http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#instance-method-timestamp

That's weird. The RxJS4 to RxJS5 migration guide actually covers this one specifically:

https://github.com/ReactiveX/rxjs/blob/master/MIGRATION.md

I remember it not working at some point, but perhaps it got re-added? Will try because it makes the example a lot simpler and adds "wow factor" 😄

It seems it works but need to map it to a plain object now: https://codesandbox.io/s/vn9qjnqovl

@jseminck Hm... I'm thinking maybe we can relax the check here:

rx-connect/src/rxConnect.js

Lines 67 to 78 in e586b5e

.scan((state, mutation) => {

if (isPlainObject(mutation)) {

return Object.assign({}, state, mutation);

}

if (typeof mutation === "function") {

return mutation(state);

}

// eslint-disable-next-line no-console

console.error(`Mutation must be a plain object or function. Check rxConnect of ${getDisplayName(WrappedComponent)}. Got: `, mutation);

return state;

i.e. check if it's function first, if not - do the assignment. WDYT?

Yeah, will try it 😄

It doesn't really fix the issue here. .timestamp() returns an object, just not a plain object. From the docs: Observable<Timestamp>

What seems to work is adding this (along with lodash.isobject). But I don't know what other implications it has ...

if (isPlainObject(mutation)) { return Object.assign({}, state, mutation); } if (isObject(mutation)) { return Object.assign({}, state, {...mutation}); }

sounds good to me :)

I'll try it and run my branch on our code, to see if things still work 👍

bsideup · 2017-09-05T07:33:14Z

README.md

+
+## Using Rx4?
+
+This library supports Rx5 by default, but provides an adapter for Rx4:


library's actual name is "RxJS" :)

bsideup · 2017-09-05T07:36:41Z

examples/blog/src/actions/posts.js

-            .getJSON(`//jsonplaceholder.typicode.com/posts?${ userId ? "userId=" + userId : ""}`)
+        return Rx.Observable
+            .ajax(`//jsonplaceholder.typicode.com/posts?${ userId ? "userId=" + userId : ""}`)
+            .map(e => e.response)


.pluck("response")?

bsideup · 2017-09-05T07:37:59Z

examples/blog/src/view/LoginView.jsx

 import { connect } from "react-redux";
 import wrapActionCreators from "react-redux/lib/utils/wrapActionCreators";
-import { rxConnect, ofActions } from "rx-connect";
+import { rxConnect, ofActions } from "../../../../src";


Check Webpack's config and alias section. from "rx-connect" was aliasing to the sources actually

Ah cool. Yeah that's much nicer. I was confused because rx-connect is also specified as a dependency in the package.json: https://github.com/bsideup/rx-connect/blob/master/examples/blog/package.json#L19

bsideup · 2017-09-05T07:38:39Z

examples/blog/src/view/Time.jsx

 )
 export default class Time extends React.PureComponent {
    render() {
-        return <div>{new Date(this.props.timestamp).toLocaleTimeString()}</div>
+        return <div>{this.props.value} {new Date(this.props.timestamp).toLocaleTimeString()}</div>


{this.props.value} ?

Was testing stuff.. :D

bsideup · 2017-09-05T07:39:28Z

npm-debug.log

@@ -0,0 +1,48 @@
+0 info it worked if it ends with ok


I had to modify getAdapter() in rx-connect --- it was returning as an esModule probably because of the way it was being bundled through webpack...

jseminck · 2017-09-05T18:27:55Z

Tackled comments. Updated some examples. Also made examples on Code Sandbox with the current dependency but by changing the adapter: https://codesandbox.io/s/vn9qjnqovl

Some things...

Timestamp

Seems to work, but need to map to a plain object

@rxConnect(
  Rx.Observable
    .timer(0, 1000)
    .timestamp()
    .map(timestamp => ({ ...timestamp }))
)

JSONP

It seems RxJS5 doesn't have JSONP support out of the box, which is needed for the Wikipedia examples. See ReactiveX/rxjs#2095 & ReactiveX/rxjs#1223

As a workaround:

// RxJS5 doesn't support JSONP out of the box
function fetchFromWikipedia(search) {
  return fetchJsonp(
    `https://en.wikipedia.org/w/api.php?action=opensearch&search=${search}&format=json&callback=JSONPCallback`
  ).then(response => response.json());
}

function searchWikipedia(search) {
  return (
    Rx.Observable
      .from(fetchFromWikipedia(search))
      // Wikipedia has really weird response format o_O
      .map(([, titles, , urls]) =>
        titles.map((title, i) => ({ title, url: urls[i] }))
      )
  );
}

Maybe we can find a better example that doesn't need JSONP and we can use plain ajax?

getAdapter()

After changing the example to use webpack alias, I had to change getAdapter(). I think there's some issue in the build process for the blog example.

export function getAdapter() {
    const adapter = rxConnect.adapter || require("./rx5Adapter");
    return adapter.__esModule ? adapter.default : adapter;
}

I'll go over the examples once more later, probably still missed some stuff... The main differences are:

flatMapLatest to switchMap
debounce to debounceTime
.jsonp to ajax
The timestamp thing

jseminck · 2017-09-20T05:18:57Z

@bsideup have you seen my comment @ #9 (comment) ?

Otherwise, what else do we need to do to get this in? Would love to have this 😄

bsideup · 2017-09-20T06:22:22Z

README.md

@@ -53,7 +53,7 @@ with this:
 import { rxConnect } from "rx-connect";

 @rxConnect(
-    Rx.Observable.timer(0, 1000).timestamp()


sounds good to me :)

bsideup · 2017-09-20T06:23:00Z

examples/blog/src/actions/users.js


 export function fetchUsers() {
    return () => {
-        return Rx.DOM.getJSON(`//jsonplaceholder.typicode.com/users`);
+        return Rx.Observable.ajax(`//jsonplaceholder.typicode.com/users`).map(e => e.response);


bsideup · 2017-09-20T06:24:51Z

@jseminck yeah, sorry for missing that. I left one comment re plain objects and minor one about pluck. Happy to merge once at least the first one is addressed :)

bsideup · 2017-09-21T06:29:02Z

@jseminck nice! Please also update the examples (Timers) and we're ready to go 🎉

jseminck · 2017-09-21T06:30:50Z

Just pushed this now to run it with our project & see if everything remains green. Will update examples during the day.

…

On Thu, 21 Sep 2017 at 09:29, Sergei Egorov ***@***.***> wrote: @jseminck <https://github.com/jseminck> nice! Please also update the examples (Timers) and we're ready to go 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJJI0U5cXXL_FCMfOuoXceeCRgW8soVrks5skgIugaJpZM4PLJH3> .

bsideup · 2017-09-25T05:52:20Z

@jseminck merged! Thank a lot for your contribution :) Will release it in a few minutes

jseminck · 2017-09-25T05:57:45Z

I will test it further, and update code sandbox to use the latest version too!

…

On Mon, 25 Sep 2017 at 08:52, Sergei Egorov ***@***.***> wrote: @jseminck <https://github.com/jseminck> merged! Thank a lot for your contribution :) Will release it in a few minutes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJJI0RBtDt-GljtxiXpNDUhJzbF5AGs9ks5slz-UgaJpZM4PLJH3> .

bsideup · 2017-09-25T07:39:00Z

@jseminck released as 0.7.0 🎉

I'll update CodeSandbox and remove JSONP :)

bsideup · 2017-09-25T07:44:39Z

@jseminck done :) Thanks again :)

jseminck added 5 commits September 3, 2017 12:49

Switch to using rx5 as default

669e7d8

Update readme to use Rx5 example. Remove the link to codepen that was…

5cb3145

…n't being shown

Update docs to explain how to use Rx4

aa97b5a

Update blog example

d22906a

Update wikistream example

5834708

bsideup requested changes Sep 5, 2017

View reviewed changes

jseminck added 6 commits September 5, 2017 20:45

Update Readme

9e690a5

Use pluck

d982229

Use webpack alias to rx-connect

90ed58f

I had to modify getAdapter() in rx-connect --- it was returning as an esModule probably because of the way it was being bundled through webpack...

Return npm-debug.log

976c6c4

Update some docs

131ae37

Remove console log

8aa12e8

Merge branch 'master' into rx5-by-default

77d392d

bsideup reviewed Sep 20, 2017

View reviewed changes

Joachim Seminck added 2 commits September 21, 2017 08:37

Accept normal objects which are not arrays

a2d326c

Pluck pluck pluck!! :)

3e0416d

Update timestamp() examples

d16a6d3

bsideup approved these changes Sep 25, 2017

View reviewed changes

bsideup merged commit f2bff11 into bsideup:master Sep 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Rx5 by default #9

Use Rx5 by default #9

jseminck commented Sep 3, 2017

jseminck commented Sep 3, 2017 •

edited

Loading

bsideup commented Sep 4, 2017

bsideup commented Sep 4, 2017

bsideup Sep 5, 2017

jseminck Sep 5, 2017

jseminck Sep 5, 2017

bsideup Sep 5, 2017

jseminck Sep 5, 2017

jseminck Sep 7, 2017 •

edited

Loading

bsideup Sep 20, 2017

jseminck Sep 20, 2017

bsideup Sep 5, 2017

bsideup Sep 5, 2017

bsideup Sep 5, 2017

jseminck Sep 5, 2017

bsideup Sep 5, 2017

jseminck Sep 5, 2017

bsideup Sep 5, 2017

jseminck commented Sep 5, 2017 •

edited

Loading

jseminck commented Sep 20, 2017

bsideup Sep 20, 2017

bsideup Sep 20, 2017

bsideup commented Sep 20, 2017

bsideup commented Sep 21, 2017

jseminck commented Sep 21, 2017 via email

bsideup commented Sep 25, 2017

jseminck commented Sep 25, 2017 via email

bsideup commented Sep 25, 2017

bsideup commented Sep 25, 2017

	.scan((state, mutation) => {
	if (isPlainObject(mutation)) {
	return Object.assign({}, state, mutation);
	}

	if (typeof mutation === "function") {
	return mutation(state);
	}

	// eslint-disable-next-line no-console
	console.error(`Mutation must be a plain object or function. Check rxConnect of ${getDisplayName(WrappedComponent)}. Got: `, mutation);
	return state;


		## Using Rx4?

		This library supports Rx5 by default, but provides an adapter for Rx4:

Use Rx5 by default #9

Use Rx5 by default #9

Conversation

jseminck commented Sep 3, 2017

jseminck commented Sep 3, 2017 • edited Loading

bsideup commented Sep 4, 2017

bsideup commented Sep 4, 2017

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

jseminck Sep 7, 2017 • 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

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

jseminck commented Sep 5, 2017 • edited Loading

jseminck commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup commented Sep 20, 2017

bsideup commented Sep 21, 2017

jseminck commented Sep 21, 2017 via email

bsideup commented Sep 25, 2017

jseminck commented Sep 25, 2017 via email

bsideup commented Sep 25, 2017

bsideup commented Sep 25, 2017

jseminck commented Sep 3, 2017 •

edited

Loading

jseminck Sep 7, 2017 •

edited

Loading

jseminck commented Sep 5, 2017 •

edited

Loading