Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
errors,stream-transform: migrate to use internal/errors.js #13310
Conversation
|
|
||
| if (ts.transforming) | ||
| throw new Error('Calling transform done when still transforming'); | ||
| throw new errors.Error('ERR_CALLING_IN_TRANSFORMING', | ||
| 'Calling transform done when still transforming'); |
kunalspathak
May 30, 2017
Member
'Calling transform done when still transforming' [](start = 27, length = 48)
don't need this anymore?
'Calling transform done when still transforming' [](start = 27, length = 48)
don't need this anymore?
| @@ -8,7 +8,8 @@ const stream = new Transform({ | |||
|
|
|||
| stream.on('error', common.mustCall((err) => { | |||
| assert.strictEqual(err.toString(), | |||
| 'Error: write callback called multiple times'); | |||
| 'Error [ERR_MULTIPLE_WRITE_CALLBACK]:' + | |||
| ' write callback called multiple times'); | |||
kunalspathak
May 30, 2017
Member
[](start = 22, length = 1)
nit: can you move the space on above line?
[](start = 22, length = 1)
nit: can you move the space on above line?
| @@ -139,6 +142,7 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected'); | |||
| E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe'); | |||
| E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks'); | |||
| E('ERR_MISSING_ARGS', missingArgs); | |||
| E('ERR_MULTIPLE_WRITE_CALLBACK', 'write callback called multiple times'); | |||
jasnell
May 30, 2017
Member
If I recall correctly, there are a couple of places in core with a similar error. A more generic error message may be appropriate.
If I recall correctly, there are a couple of places in core with a similar error. A more generic error message may be appropriate.
|
@sreepurnajasti - Changes incorporated from my feedback looks good although I kind of don't like |
| @@ -217,10 +218,10 @@ function done(stream, er, data) { | |||
| var ts = stream._transformState; | |||
|
|
|||
| if (ws.length) | |||
| throw new Error('Calling transform done when ws.length != 0'); | |||
| throw new errors.Error('ERR_CALLING_WS_LENGTH'); | |||
mhdawson
May 31, 2017
Member
I'd agree a better name for the error would be good. Maybe 'ERR_TRANSFORM_WITH_LENGTH_0'.
I'd agree a better name for the error would be good. Maybe 'ERR_TRANSFORM_WITH_LENGTH_0'.
| @@ -90,7 +91,7 @@ function afterTransform(stream, er, data) { | |||
|
|
|||
| if (!cb) { | |||
| return stream.emit('error', | |||
| new Error('write callback called multiple times')); | |||
| new errors.Error('ERR_MULTIPLE_CALLBACK')); | |||
mhdawson
May 31, 2017
Member
Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_MULTIPLE_CALLBACK'.
Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_MULTIPLE_CALLBACK'.
|
|
||
| if (ts.transforming) | ||
| throw new Error('Calling transform done when still transforming'); | ||
| throw new errors.Error('ERR_CALLING_IN_TRANSFORMING'); |
mhdawson
May 31, 2017
Member
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.
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.
|
CI good landing |
|
Landed as d50a802 |
PR-URL: #13310 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
@mhdawson Thank you :) |
|
I'm strongly -1 on this one. This forces some good rework on |
|
To be clear, this is something we should be doing asap, but we need to think a bit how we want to do it. |
|
:-/ ... 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... |
|
That's ok for me, there is no hurry to revert. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
lib/_stream_transform.js
ref: #11273