Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to RxJS 5.0.0-alpha.6 #21

Merged
merged 4 commits into from
Oct 21, 2015
Merged

Update to RxJS 5.0.0-alpha.6 #21

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a16a36
Rx 5.0.0-alpha.6
TylorS Oct 20, 2015
ba1512b
Fixed last failing test
TylorS Oct 21, 2015
dd99b98
fixes
TylorS Oct 21, 2015
fec4475
Use publishReplay(1)
TylorS Oct 21, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@
"superagent": "1.4.0"
},
"peerDependencies": {
"@cycle/core": "5.x.x"
"@reactivex/rxjs": "5.0.0-alpha.6"
},
"devDependencies": {
"@cycle/core": "5.0.x",
"rx": "4.0.6",
"@reactivex/rxjs": "5.0.0-alpha.6",
"babel": "5.8.23",
"babelify": "6.3.0",
"body-parser": "1.14.1",
Expand Down
27 changes: 15 additions & 12 deletions src/http-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const Rx = require(`rx`)
const Rx = require(`@reactivex/rxjs`)
const superagent = require(`superagent`)

function optionsToSuperagent({
Expand Down Expand Up @@ -71,24 +71,29 @@ function createResponse$(reqOptions) {
if (typeof reqOptions === `string`) {
request = urlToSuperagent(reqOptions)
} else if (typeof reqOptions === `object`) {
request = optionsToSuperagent(reqOptions)
try {
request = optionsToSuperagent(reqOptions)
} catch (err) {
observer.error(err)
return () => {} // noop
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks actually like a proper solution IMO.

} else {
observer.onError(new Error(`Observable of requests given to HTTP ` +
observer.error(new Error(`Observable of requests given to HTTP ` +
`Driver must emit either URL strings or objects with parameters.`))
return () => {} // noop
}

try {
request.end((err, res) => {
if (err) {
observer.onError(err)
observer.error(err)
} else {
observer.onNext(res)
observer.onCompleted()
observer.next(res)
observer.complete()
}
})
} catch (err) {
observer.onError(err)
observer.error(err)
}

return function onDispose() {
Expand All @@ -103,14 +108,12 @@ function makeHTTPDriver({eager = false} = {eager: false}) {
.map(reqOptions => {
let response$ = createResponse$(reqOptions)
if (eager || reqOptions.eager) {
response$ = response$.replay(null, 1)
response$.connect()
response$ = response$.publishReplay(null, 1).connect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha this shouldn't work, because connect() actually returns a Subscription, not an Observable. That's why in the original code the connect() was on a different line.

return response$
}
response$.request = reqOptions
return response$
})
.replay(null, 1)
response$$.connect()
}).shareReplay(null, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shareReplay is replay().refCount(), which is different than replay() + connect(). I'd advise to keep the replay() + connect().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same line or separate like above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every tests fails switching to publishReplay(null, 1) + connect()
With this error:

Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be related to ReactiveX/rxjs#453, so you can revert it so tests pass. But I'm not particularly happy with this situation. Let's make tests pass and then later let's try using HTTP driver with RxJS Next in real apps to see if there is actually a problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response$$ wth publishReplay(1) + connect() 100% does not work. Yes I realized that it shouldn't have a an observable as it's value, using and empty {} seems to work though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all tests fail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All HTTP tests, the node tests pass, unless for whatever reason I use response$$.shareReplay(1)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send a commit using publishReplay(1) + connect()? I want to check why they are failing. We can figure out the tests later on with another PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return response$$
}
}
Expand Down
37 changes: 19 additions & 18 deletions test/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
/* global describe, it */
var assert = require('assert');
var src = require('../lib/index');
var Cycle = require('@cycle/core');
var Rx = require('rx');
var Rx = require('@reactivex/rxjs');
var makeHTTPDriver = src.makeHTTPDriver;

function run(uri) {
Expand All @@ -18,12 +17,13 @@ function run(uri) {
describe('HTTP Driver', function () {
it('should throw when request stream emits neither string nor object',
function(done) {
var request$ = Rx.Observable.just(123);
var request$ = Rx.Observable.of(123);
var httpDriver = makeHTTPDriver();
var response$$ = httpDriver(request$);
response$$.mergeAll().subscribe(
function onNext() { assert.fail(); },
function onError(err) {
debugger;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? :)

assert.strictEqual(err.message, 'Observable of requests given to ' +
'HTTP Driver must emit either URL strings or objects with ' +
'parameters.'
Expand All @@ -36,25 +36,26 @@ function run(uri) {

it('should throw when given options object without url string',
function(done) {
var request$ = Rx.Observable.just({method: 'post'});
var request$ = Rx.Observable.of({method: 'post'});
var httpDriver = makeHTTPDriver();
var response$$ = httpDriver(request$);
response$$.mergeAll().subscribe(
function onNext() { assert.fail(); },
function onError(err) {
assert.strictEqual(
err.message, 'Please provide a `url` property in the request ' +
'options.'
);
done();
}
);
response$$.mergeAll()
.subscribe(
function onNext() { assert.fail(); },
function onError(err) {
assert.strictEqual(
err.message, 'Please provide a `url` property in the request ' +
'options.'
);
done();
}
);
}
);

it('should return response metastream when given a simple URL string',
function(done) {
var request$ = Rx.Observable.just(uri + '/hello');
var request$ = Rx.Observable.of(uri + '/hello');
var httpDriver = makeHTTPDriver();
var response$$ = httpDriver(request$);
response$$.subscribe(function(response$) {
Expand All @@ -70,7 +71,7 @@ function run(uri) {

it('should return response metastream when given simple options obj',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/pet',
method: 'POST',
send: {name: 'Woof', species: 'Dog'}
Expand All @@ -93,7 +94,7 @@ function run(uri) {

it('should return response metastream when given another options obj',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/querystring',
method: 'GET',
query: {foo: 102030, bar: 'Pub'}
Expand All @@ -117,7 +118,7 @@ function run(uri) {

it('should send 500 server errors to response$ onError',
function(done) {
var request$ = Rx.Observable.just(uri + '/error');
var request$ = Rx.Observable.of(uri + '/error');
var httpDriver = makeHTTPDriver();
var response$$ = httpDriver(request$);
response$$.subscribe(function(response$) {
Expand Down
11 changes: 5 additions & 6 deletions test/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@ require('./common')(uri);

// Node.js specific ============================================================
var assert = require('assert');
var Cycle = require('@cycle/core');
var Rx = require('rx');
var Rx = require('@reactivex/rxjs');
var src = require('../lib/index');
var makeHTTPDriver = src.makeHTTPDriver;
var globalSandbox = require('./support/global');

describe('HTTP Driver in Node.js', function () {
it('should auto-execute HTTP request when factory gets eager = true',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/pet',
method: 'POST',
send: {name: 'Woof', species: 'Dog'}
Expand All @@ -35,7 +34,7 @@ describe('HTTP Driver in Node.js', function () {

it('should not auto-execute HTTP request by default',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/pet',
method: 'POST',
send: {name: 'Woof', species: 'Dog'}
Expand All @@ -52,7 +51,7 @@ describe('HTTP Driver in Node.js', function () {

it('should auto-execute HTTP request if the request has eager = true',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/pet',
method: 'POST',
send: {name: 'Woof', species: 'Dog'},
Expand All @@ -72,7 +71,7 @@ describe('HTTP Driver in Node.js', function () {

it('should not auto-execute HTTP request when factory gets eager = false',
function(done) {
var request$ = Rx.Observable.just({
var request$ = Rx.Observable.of({
url: uri + '/pet',
method: 'POST',
send: {name: 'Woof', species: 'Dog'}
Expand Down