strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Add object mode ReadableStream support #216

Closed ritch closed 9 years ago

ritch commented 9 years ago

This PR adds support for returning object mode ReadableStreams.

connect to strongloop/strong-remoting#214

Using HTTP ReadableStreams are piped to the res using https://github.com/dominictarr/mux-demux. This allows for stream.on('error')s to be sent using the multiplexed NL separated JSON protocol.

I would still like to add the same exact capability using multipart responses.

/to @bajtos /cc @raymondfeng @kraman @rmg (noticed you had used mux-demux)

rmg commented 9 years ago

Missing JSONStream dependency.. also, please consider switching to jsonstream instead - it's the canonical version of the module. Sadly, since OS X uses a case-folding filesystem, it can't actually tell the difference between them :-(

bajtos commented 9 years ago

@ritch do you expect any non-javascript consumers of this new stream API? If you do, then could you please add a unit-test showing how to read and parse the stream using plain REST?

The Angular code generator will not support this new stream type OOTB, do you have any plans for that SDK? Should it ignore (skip) any methods that return a stream?

bajtos commented 9 years ago

@ritch I am assigning this back to you, please let me know when the code is ready for the final review.

ritch commented 9 years ago

This should be ready to merge @bajtos

bajtos commented 9 years ago

@ritch the CI is red (both Travis and Jenkins), PTAL.

ritch commented 9 years ago

test please

bajtos commented 9 years ago

@ritch It seems to me that the commit "Merge and fix iojs failures" is bringing in a bunch of changes already landed on the master branch. Would you mind rebasing this PR on the current master, so that 1) the history is linear 2) the diff on github is not polluted with changes that were already reviewed and landed?

ritch commented 9 years ago

@bajtos I'm not going to rewrite the history. It takes forever and is error prone (since the commits conflict on the package deps and dev deps.

This PR has gone on way longer than is practical. Just look at the diff instead of the commits.

ritch commented 9 years ago

@bajtos ...actually let me show you the pain I am going through when rebasing and perhaps you can either see why I am refusing... or you might be able to see what I am doing incorrectly and prevent future pain.

Here is what I am doing:

git fetch # ensure we have latest from master
git rebase master -i # so we can squash the commits

# outputs...
pick 6d9ba85 Add object mode ReadableStream support
pick 4038439 Add support for Event Source streams

# I change the second "pick" to "s" to squash it into the previous commit
# note the merge commit doesn't show up...

# but git is unable to perform the rebase:

could not apply 6d9ba85... Add object mode ReadableStream support

If I don't try to squash the commit, the rebase outputs:

First, rewinding head to replay your work on top of it...
Applying: Add object mode ReadableStream support
Using index info to reconstruct a base tree...
M   lib/http-context.js
M   lib/rest-adapter.js
M   lib/shared-method.js
M   package.json
M   test/streams.js
Falling back to patching base and 3-way merge...
Auto-merging test/streams.js
Auto-merging package.json
**CONFLICT (content): Merge conflict in package.json**
Auto-merging lib/shared-method.js
Auto-merging lib/rest-adapter.js
Auto-merging lib/http-context.js
**CONFLICT (content): Merge conflict in lib/http-context.js**
Failed to merge in the changes.
Patch failed at 0001 Add object mode ReadableStream support

Which I can resolve manually. Its just pretty tedious and easy to merge incorrectly since the diff looks like this:

  "dependencies": {
<<<<<<< HEAD
    "express": "4.x",
    "body-parser": "^1.12.4",
    "debug": "^2.2.0",
    "eventemitter2": "^0.4.14",
    "cors": "^2.6.0",
    "jayson": "^1.2.0",
    "js2xmlparser": "^0.1.9",
    "async": "^0.9.0",
    "traverse": "^0.6.6",
    "request": "^2.55.0",
    "qs": "^2.4.2",
    "inflection": "^1.7.1",
    "xml2js": "^0.4.8"
  },
  "devDependencies": {
    "bluebird": "^2.9.25",
    "browserify": "~10.2.0",
    "chai": "^2.3.0",
=======
    "async": "^0.9.0",
    "body-parser": "^1.7.0",
    "cors": "^2.5.2",
    "debug": "^2.0.0",
    "eventemitter2": "^0.4.14",
    "express": "4.x",
    "inflection": "^1.4.2",
    "jayson": "^1.1.1",
    "js2xmlparser": "^0.1.3",
    "mux-demux": "^3.7.9",
    "qs": "^2.2.3",
    "request": "^2.42.0",
    "traverse": "^0.6.6",
    "xml2js": "^0.4.4"
  },
  "devDependencies": {
    "bluebird": "^2.9.6",
    "browserify": "~5.11.1",
    "chai": "^1.10.0",
    "event-stream": "^3.3.1",
>>>>>>> Add object mode ReadableStream support

Anyways... I don't need help rebasing... I can get all of that to work. Also I am not against rebasing in general or rewriting history. I'm just tired of re-merging the same changes over and over again only to have missed a version number, have that fail the build on ci and then spend time finding that mistake.

So if you have any way to make this a bit easier or less error prone I would love to know.

bajtos commented 9 years ago

So if you have any way to make this a bit easier or less error prone I would love to know.

I am afraid I don't have any better solution, I think this is the inherent cost of long-living feature branches :(

Just look at the diff instead of the commits.

That's what I did. The diff on github includes many unrelated changes, so it's rather difficult to distinguish signal from noise. Fortunately git client shows shorter diff, I'll review using that way.

bajtos commented 9 years ago

Your package.json changes are downgrading certain dependencies, I pointed out two cases in my comments above. Could you please double-check other deps too?

bajtos commented 9 years ago

The code looks good otherwise, merge at will.

rmg commented 9 years ago

The rebase conflicts in lib/http-context.js are because of the merging of master into the branch instead of frequent rebasing to keep up.

The rebase conflicts in package.json are because master doesn't list the dependencies in alphabetical order but your commit that added 2 dependencies re-ordered them. This is where git add -p is your friend, since it allows you to commit the re-ordering as a separate commit from adding the deps.

As for re-writing history... A PR is a patch set proposal, and rewriting its history is more akin to working with an editor and revising a manuscript before sending it to the printers where it does become part of the canonical history. This is where rebasing is a good thing.

rmg commented 9 years ago

The numerous dependency downgrades are the result of the merge commit discarding the changes to package.json that were made to master since this branch started.

bajtos commented 9 years ago

It looks to me like not rewriting the history is even more error prone that rewriting it.

ritch commented 9 years ago

So I guess rewriting history itself is not the issue. Its manual merging. Merging is error prone and tedious. It sounds like the only way to avoid having to manually merge files is to rebase often (thanks for pointing that out @rmg).

ritch commented 9 years ago

test please