search

strongloop / strong-remoting

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

Fixes the test/streams.js test #219

Closed jorrit closed 8 years ago

jorrit commented 9 years ago

Broken since f01b8a6ba0b157f3e6562d11158bc3050a75d61a

slnode commented 9 years ago

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

jorrit commented 9 years ago

The patch is not yet finished, I need to add uri encoding for the pameters.

jorrit commented 9 years ago

It is finished, I squashed the two commits.

jorrit commented 9 years ago

The waiting label can be removed.

rmg commented 9 years ago

ok to test

bajtos commented 9 years ago

@ritch you were working on the stream support in strong-remoting recently, thus I think you are the best person to review this patch?

bajtos commented 8 years ago

@jorrit sorry for the long delay in our review process. Are you still keen to get this patch finished?

bajtos commented 8 years ago

Actually, I am quite puzzled by this. AFAICT, all tests are passing on the current master, which makes me think this patch is no longer relevant.

@jorrit @ritch thoughts?

jorrit commented 8 years ago

I think the node api has changed. At the time I created the PR, node 0.12 just changed the api of fs.createReadStream so the second argument was an object instead of a string. I recall that this caused the problem that led me to create the PR. However, when looking at the docs of the current node version, it seems that both a string and an object are allowed. So this PR is no longer needed.