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.

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

Bring string_decoder into the foundation #272

Closed
mcollina opened this issue Apr 4, 2017 · 12 comments
Closed

Bring string_decoder into the foundation #272

mcollina opened this issue Apr 4, 2017 · 12 comments

Comments

@mcollina
Copy link
Member

mcollina commented Apr 4, 2017

Very recently a bug was reported to string_decoder, and @rvagg forwarded it to me. (a test is failing in node 4+).

That module uses a very similar machinery to readable-stream to keep it in sync with latest Node.js. I'm in the process of updating that as well to the latest node.js.

Speaking with @rvagg, I think it might be a good idea to move it under the nodejs github organization, and we keep that in sync together with readable-stream.

Do you all agree it's a good idea?

@calvinmetcalf
Copy link
Contributor

is this the one browserify uses ?

@mcollina
Copy link
Member Author

mcollina commented Apr 4, 2017

@mcollina
Copy link
Member Author

mcollina commented Apr 4, 2017

@calvinmetcalf
Copy link
Contributor

we also rely on process, that might be a good one to move over to the node umbrella too, that being said it's very different from string_decoder or this as it's a shim that attempts to simulate node behavior

@mcollina
Copy link
Member Author

mcollina commented Apr 4, 2017

@calvinmetcalf I think string_decoder is lifted from core, and as such I think it should be under the foundation.

process is very different, I would not touch that atm.

@mcollina
Copy link
Member Author

mcollina commented May 3, 2017

I've tagged this @nodejs/ctc, as I think it's the best course of action.

@joshgav
Copy link

joshgav commented May 10, 2017

This should be reviewed by the TSC too, added label.

@Trott Trott removed the ctc-agenda label May 10, 2017
@mcollina
Copy link
Member Author

@nodejs/ctc has no problem in this if @nodejs/tsc is ok with it.

@mhdawson
Copy link
Member

This looks familiar was there another issue where people were asked to weigh in or was that a different package ?

@joshgav
Copy link

joshgav commented May 18, 2017

@mhdawson

ref: nodejs/TSC#260

@mhdawson
Copy link
Member

@joshgav thanks, wanted to make sure I had chimed into the conversation.

@mcollina
Copy link
Member Author

This has been done, and we can close this.

@Trott Trott removed the tsc-agenda label Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants