GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — errors,stream-transform: migrate to use internal/errors.js by sreepurnajasti · Pull Request #13310 · nodejs/node

Conversation

@sreepurnajasti

@sreepurnajasti sreepurnajasti commented May 30, 2017

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originating in Node.js core. stream Issues and PRs related to Node.js streams. labels May 30, 2017
kunalspathak
kunalspathak reviewed May 30, 2017
View reviewed changes
Comment thread lib/_stream_transform.js Outdated

@kunalspathak kunalspathak May 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sreepurnajasti sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kunalspathak
kunalspathak reviewed May 30, 2017
View reviewed changes
Comment thread test/parallel/test-stream-transform-callback-twice.js Outdated

@kunalspathak kunalspathak May 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sreepurnajasti sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@kunalspathak Done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell
jasnell reviewed May 30, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

@jasnell jasnell May 30, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sreepurnajasti sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sreepurnajasti
sreepurnajasti force-pushed the stream-transform branch from 8ff453c to 404387b Compare May 31, 2017 12:25
@kunalspathak

kunalspathak commented May 31, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson
mhdawson reviewed May 31, 2017
View reviewed changes
Comment thread lib/_stream_transform.js Outdated

@mhdawson mhdawson May 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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'.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell and sreepurnajasti reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@sreepurnajasti sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@mhdawson Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_stream_transform.js Outdated

@mhdawson mhdawson May 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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'.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell and sreepurnajasti reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@sreepurnajasti sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@mhdawson Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_stream_transform.js Outdated

@mhdawson mhdawson May 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell and sreepurnajasti reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@sreepurnajasti sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@mhdawson Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sreepurnajasti
sreepurnajasti force-pushed the stream-transform branch from 404387b to 47ada2c Compare June 12, 2017 15:17
@sreepurnajasti

sreepurnajasti commented Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jun 13, 2017
View reviewed changes
@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
sreepurnajasti @sreepurnajasti
errors,stream-transform: migrate to use internal/errors.js
9ba9855
@sreepurnajasti
sreepurnajasti force-pushed the stream-transform branch from 0cc5f2d to 9ba9855 Compare June 14, 2017 15:24
mhdawson
mhdawson approved these changes Jun 14, 2017
View reviewed changes
@mhdawson

mhdawson commented Jun 14, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Jun 15, 2017

Copy link
Copy Markdown
Member

CI good landing
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Jun 15, 2017

Copy link
Copy Markdown
Member

Landed as d50a802
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhdawson pushed a commit that referenced this pull request Jun 15, 2017
sreepurnajasti @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

sreepurnajasti commented Jun 18, 2017

Copy link
Copy Markdown
Contributor Author

@mhdawson Thank you :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jun 21, 2017

Copy link
Copy Markdown
Member

:-/ ... 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...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
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
+2 more reviewers
@kunalspathak kunalspathak kunalspathak left review comments
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originating 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 Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@sreepurnajasti @kunalspathak @mhdawson @mcollina @jasnell @TimothyGu @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.