FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

string_decoder: Migrate to use internal/errors by starkwang · Pull Request #14682 · nodejs/node · GitHub

/ node Public

string_decoder: Migrate to use internal/errors - #14682

Closed
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:string-decoder-internal-errors
ConversationCommits (1)ChecksFiles changed
Closed

string_decoder: Migrate to use internal/errors#14682
starkwang wants to merge 1 commit into
nodejs:masterfrom
starkwang:string-decoder-internal-errors

Conversation

starkwang commented Aug 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot added the string_decoder Issues and PRs related to the string_decoder subsystem. label Aug 8, 2017
refack self-assigned this Aug 8, 2017
jasnell mentioned this pull request Aug 8, 2017
Closed
80 tasks
refack added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 8, 2017

refack commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/9538/

BridgeAR reviewed Aug 8, 2017
View reviewed changes
Comment thread lib/string_decoder.js Outdated

BridgeAR Aug 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

starkwang force-pushed the string-decoder-internal-errors branch from ed34f34 to 660dc96 Compare August 8, 2017 06:21

refack commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

ping @nodejs/ctc, needs approval

mcollina commented Aug 8, 2017

Copy link
Copy Markdown
Member

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 added blocked PRs that are blocked by other issues or PRs. stream Issues and PRs related to the stream subsystem. labels Aug 8, 2017

refack commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

blocked until we have a modular solution for readable-stream

mcollina removed the blocked PRs that are blocked by other issues or PRs. label Sep 12, 2017

mcollina commented Sep 12, 2017

Copy link
Copy Markdown
Member

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)

mcollina approved these changes Sep 12, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if CI green.

BridgeAR commented Sep 13, 2017

Copy link
Copy Markdown
Member

Landed in eb4940e

BridgeAR closed this Sep 13, 2017
BridgeAR pushed a commit that referenced this pull request Sep 13, 2017
string_decoder: Migrate to use internal/errors
eb4940e 
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 pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
string_decoder: Migrate to use internal/errors
a08bf62 
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
string_decoder: Migrate to use internal/errors
c62ac2f 
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 mentioned this pull request Oct 30, 2017
Merged
refack removed their assignment Oct 20, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
BridgeAR BridgeAR approved these changes
+2 more reviewers
refack refack approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. string_decoder Issues and PRs related to the string_decoder subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL