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

string_decoder: Migrate to use internal/errors #14682

Closed

Conversation

@starkwang
Copy link
Contributor

@starkwang starkwang commented Aug 8, 2017

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

string_decoder

@refack
refack approved these changes Aug 8, 2017
@refack refack self-assigned this Aug 8, 2017
@jasnell jasnell mentioned this pull request Aug 8, 2017
79 of 80 tasks complete
lib/string_decoder.js Outdated
@@ -31,7 +32,7 @@ function normalizeEncoding(enc) {
const nenc = internalUtil.normalizeEncoding(enc);
if (typeof nenc !== 'string' &&
(Buffer.isEncoding === isEncoding || !Buffer.isEncoding(enc)))
throw new Error(`Unknown encoding: ${enc}`);
throw new errors.Error('ERR_UNKNOWN_ENCODING', enc);

This comment has been minimized.

@BridgeAR

BridgeAR Aug 8, 2017
Member

As this is semver-major anyway it could probably be changed to a TypeError

@starkwang starkwang force-pushed the starkwang:string-decoder-internal-errors branch to 660dc96 Aug 8, 2017
@targos
targos approved these changes Aug 8, 2017
@refack
Copy link
Member

@refack refack commented Aug 8, 2017

ping @nodejs/ctc, needs approval

@mcollina
Copy link
Member

@mcollina mcollina commented Aug 8, 2017

We ship this in readable-stream as part of http://npm.im/string_decoder. We need the same solution that we will need for the streams.

Can we hold off until that is ready? I would prefer not having to revert too many commits before releasing 9 if that is not ready.

cc @jasnell

@refack
Copy link
Member

@refack refack commented Aug 8, 2017

blocked until we have a modular solution for readable-stream

@refack refack added this to In Progress in Error Codes Aug 20, 2017
@mcollina mcollina removed the blocked label Sep 12, 2017
@mcollina
Copy link
Member

@mcollina mcollina commented Sep 12, 2017

At the latest streams wg we decided to unblock this. We would like this (and the equals for Writable, Duplex and Transform) to ship in Node 9.

Ref: nodejs/readable-stream#309 (comment)

Copy link
Member

@mcollina mcollina left a comment

LGTM if CI green.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 13, 2017

Landed in eb4940e

@BridgeAR BridgeAR closed this Sep 13, 2017
BridgeAR added a commit that referenced this pull request Sep 13, 2017
PR-URL: #14682
Refs: #11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
PR-URL: nodejs/node#14682
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
PR-URL: nodejs/node#14682
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell jasnell moved this from In Progress to Done in Error Codes Oct 25, 2017
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.