bring string_decoder into the Foundation

[mcollina]

Requesting approval to move https://github.com/rvagg/string_decoder to https://github.com/nodejs/string_decoder. This code has already the Node.js copyright, the only thing that is from @rvagg is the build step. This has been recently updated by myself to use the same machinery of readable-stream.

The responsibility for this module will fall under @nodejs/streams.

On approval we'd add in the Node.js governance and then transfer over.

[joshgav]

Makes sense, but could we clarify what the relationship will be between the proposed repo and lib/string_decoder.js? I think this is important so the two files don't diverge unexpectedly. Perhaps the built-in file can be vendored out and we can reference the new repo in deps, as we do for node-inspect.

Also, building this module seems to require babel and I'm a bit negative about depending on babel in core. Could it be built without babel? Thanks!

[ljharb]

It's published to npm; without babel it can't support older nodes.

[addaleax]

@joshgav I think it’s okay for the npm copy of string_decoder to support exactly what streams needs, even if they diverge (as long as the API is still the same).

(In case it’s relevant, my PR at nodejs/node#11613 would make string_decoder in core come to rely on Buffer internals that aren’t available in any currently published Node version.)

[calvinmetcalf]

@joshgav currently string decoder is built from the internal one like readable-stream does. It could probably be vendered out in a way that makes it work in current node without using babel, babel only being used to build the browserify/old node version that is put onto npm.

Currently it's created via this process

1. download it from git hub
2. run a bunch of regexes on it to get rid of node specific stuff
3. run babel with the bare minimum of rules needed for it to run in browsers/old node.

if we vendored it would could likely do away with steps 1 and 2 and just have a build process

[jasnell]

+1

[mcollina]

@joshgav in the build step, we are also importing the string_decoder tests, so any breakages would be detected.

[rvagg]

+1 obviously, since Matteo and I have had this discussion prior to raising this issue

[calvinmetcalf]

I'm +1 on this as a member of @nodejs/streams

[mhdawson]

+1 from me.

[nebrius]

I'm abstaining. I don't feel like I have enough background on this topic to give an informed vote (which is fine, it's not my wheelhouse).

[Fishrock123]

+1 based on discussion in the last CTC meeting.

[joshgav]

Thank you @addaleax and @calvinmetcalf for your explanations! I'm going to abstain cause I don't know streams well enough currently to understand the uses of this inside and outside of core; will defer to those more knowledgeable.

If no one objects I'll ask one of the transpilation experts on our TypeScript team to see if we can remove babel easily, on the assumption that we all agree fewer dependencies is better.

Thanks!

[joshgav]

@mcollina - I changed the title to make this issue easier to discover, hope you don't mind!