Stream Support #30

Merged
merged 1 commit into from Jun 22, 2012

Projects

None yet

4 participants

@sh1mmer
Contributor
sh1mmer commented Mar 26, 2012

I took another look and this could be improved but it's good enough right now

@astro
Owner
astro commented on 6559f0c Mar 24, 2012

Any reason this shouldn't get merged in?

Oh rad. Sorry I'd meant to do a pull. Let me tidy it a tiny bit first but we should definitely get streams support in there :)

@sh1mmer
Contributor
sh1mmer commented Apr 3, 2012

@astro any updates on this? It would be good to bump the version number when this gets pulled so we can depend on a stream API

@astro
Owner
astro commented Apr 9, 2012

I have a few short questions about the patch:

  • Shouldn't 'close' be emitted after 'end', not before?
  • Does it make sense to proxy the 'data' event? Counter argument: whoever holds the parser instance controls its write() too, and can emit it there if needed.
  • Why are you patching the parser's emit() function? Debugging leftover?
  • What's up with the require path modification? Is that required for node 0.4?
  • Should we do anything about error handling? node-expat is synchronous and users are already expecting exceptions to be thrown. Streams use the 'error' event.
  • Just for curiosity, do you have any particular use-case for the createParser() callback?
@tommymessbauer

node-expat is great and stream support would be awesome. Do we have a release target? Can I chip in and help in any way?

@astro
Owner
astro commented Apr 17, 2012

@tommydudebreaux That is great!

Help me with:

  • The questions above
  • Ensuring backwards compatibility (API breakage must be avoided for libraries!)
@astro
Owner
astro commented Apr 18, 2012

I am just reading that streams are going to become more simple with 0.9. Perhaps we can omit hairy things like destroySoon() from the start?

@sh1mmer
Contributor
sh1mmer commented Apr 23, 2012

Well the changes I made are already non-breaking to the existing API, I just added stuff. It still needs to be tidied a bit more, but it's functional (and I'm actually using it for a real project).

There are a few bits of expat that's I'd like to integrate better with regarding ending streams, etc.

@mvolkmann

Any update on this? I really want stream support!

@astro
Owner
astro commented May 10, 2012

I just want an opinion on my open questions. If it's different from “agree with @sh1mmer”, another branch for me to pull would help.

@astro astro merged commit 6559f0c into astro:master Jun 22, 2012
@astro
Owner
astro commented Jun 22, 2012

Alright, I took your recent commits and resolved my issues. Please tell me if I changed anything you rely on.

@astro
Owner
astro commented Jun 22, 2012

Thanks!

@sh1mmer
Contributor
sh1mmer commented Jun 22, 2012

Cool. Did you see my 1.5.0 merge?

@astro
Owner
astro commented Jun 22, 2012

Yes, though I prefer rebasing feature branches :-)

@sh1mmer
Contributor
sh1mmer commented Jun 22, 2012

Np.

On Thu, Jun 21, 2012 at 6:56 PM, Astro
reply@reply.github.com
wrote:

Yes, though I prefer rebasing feature branches :-)


Reply to this email directly or view it on GitHub:
#30 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment