Skip to content

errors,stream-transform: migrate to use internal/errors.js #13310

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

errors,stream-transform: migrate to use internal/errors.js #13310

wants to merge 1 commit into from

Conversation

@sreepurnajasti
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib/_stream_transform.js

ref: #11273

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels May 30, 2017
kunalspathak
kunalspathak reviewed May 30, 2017
View reviewed changes
lib/_stream_transform.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

'Calling transform done when still transforming' [](start = 27, length = 48)

don't need this anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kunalspathak Thanks. It's done.

kunalspathak
kunalspathak reviewed May 30, 2017
View reviewed changes
test/parallel/test-stream-transform-callback-twice.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

[](start = 22, length = 1)

nit: can you move the space on above line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kunalspathak Done.

jasnell
jasnell reviewed May 30, 2017
View reviewed changes
lib/internal/errors.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, there are a couple of places in core with a similar error. A more generic error message may be appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell Modified the error message to be more generic. Thanks

@kunalspathak
Copy link
Member

@sreepurnajasti - Changes incorporated from my feedback looks good although I kind of don't like ERR_CALLING_WS_LENGTH as the reader doesn't easily get the intent of the error message. I will leave it to @jasnell to comment on this.

mhdawson
mhdawson reviewed May 31, 2017
View reviewed changes
lib/_stream_transform.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'd agree a better name for the error would be good. Maybe 'ERR_TRANSFORM_WITH_LENGTH_0'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson Done

lib/_stream_transform.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_MULTIPLE_CALLBACK'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson Fixed.

lib/_stream_transform.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_ALREADY_TRANSFORMING' String is already specific to transform so making that part of the ID makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson Done

@sreepurnajasti
Copy link
Contributor Author

@mhdawson @jasnell As per the suggestions, it is modified. Please, have a look.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 13, 2017
@sreepurnajasti sreepurnajasti force-pushed the stream-transform branch 2 times, most recently from 47ada2c to 0cc5f2d Compare June 14, 2017 14:28
@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

CI good landing

@mhdawson
Copy link
Member

Landed as d50a802

mhdawson pushed a commit that referenced this pull request Jun 15, 2017
@mhdawson
errors,stream-transform: migrate to use internal/errors.js
d50a802 
PR-URL: #13310
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@TimothyGu TimothyGu closed this Jun 15, 2017
@sreepurnajasti
Copy link
Contributor Author

@mhdawson Thank you :)

@mcollina
Copy link
Member

I'm strongly -1 on this one. This forces some good rework on readable-stream to pull those error data. May I ask for a revert?

cc @nodejs/streams

@mcollina
Copy link
Member

To be clear, this is something we should be doing asap, but we need to think a bit how we want to do it.

@jasnell
Copy link
Member

jasnell commented Jun 21, 2017

:-/ ... reverting would be unfortunate. Perhaps instead, since this is semver-major and won't go out in a release any time soon, can we take a short bit of time to figure out the strategy for readable-stream? And if we can't identify a reasonable path forward, then revert...

@mcollina
Copy link
Member

That's ok for me, there is no hurry to revert.

@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@sreepurnajasti sreepurnajasti deleted the stream-transform branch December 27, 2017 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@kunalspathak kunalspathak kunalspathak left review comments

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sreepurnajasti @kunalspathak @mhdawson @mcollina @jasnell @TimothyGu @nodejs-github-bot