buffer: refactor buffer exports by jasnell · Pull Request #13807 · nodejs/node

jasnell · 2017-06-20T01:17:42Z

Refactor buffer exports to use module.exports = {} pattern
~~Migrate most errors to use internal/errors~~_{refack: crossed off, as this was done in errors, buffer: Migrate buffer errors to use internal/errors #13976}

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)

buffer

mscdex · 2017-06-20T03:33:27Z

Trott · 2017-06-20T06:08:46Z

lib/buffer.js currently has 100% code coverage in our test suite. If not too onerous, it would be nice to run the coverage report on this PR to make sure the refactoring doesn't introduce code paths that are now untested.

TimothyGu · 2017-06-20T02:03:22Z

I'd prefer using buf1 instead of a in the code as well.

TimothyGu · 2017-06-20T02:06:05Z

Would { FastBuffer: undefined } make it more self-documenting?

TimothyGu · 2017-06-20T02:06:30Z

Alphabetical order.

jasnell · 2017-06-20T14:36:13Z

Forgot to mark this as in progress as I still need to add the documentation updates for the new error codes...

BridgeAR · 2017-06-20T21:00:43Z

Maybe call the error type something more generic? There could be any variable that may not be negative or that may not be greater than x.

BridgeAR · 2017-06-20T21:03:26Z

I am not sure if the error codes should be more generic or not but I think it would make sense if they are. In this case the ERR_INVALID_OPT_VALUE type could also be used as such.

IMHO including the encoding name is better.

BridgeAR · 2017-06-20T21:04:34Z

See my comment below. This type is at least redundant to ERR_UNKNOWN_ENCODING but I would use ERR_INVALID_OPT_VALUE instead.

BridgeAR · 2017-06-20T21:07:41Z

I'm not sure if these types would only fit for buffers and therefore I recommend to remove the BUFFER part from the name.

There's the whole over specialization of invalid options debate:
try{// do some stuffvarf=fs.openSync('g.txt','rgh');// do other stuffcatch(e){if(e.code=='ERR_INVALID_OPT_VALUE')console.error(e)&&process.exit(1)// because it's a programming errorelse ...;}
Maybe worth creating a new type? OptionError, or have all the codes include a substring?

BridgeAR · 2017-06-20T21:11:08Z

This could be consolidated with ERR_ARG_OUT_OF_BOUNDS even though it's not as specific.

refack

Overspecialization of error codes should be discussed

refack · 2017-06-23T23:19:47Z

There's the whole over specialization of invalid options debate:
try{// do some stuffvarf=fs.openSync('g.txt','rgh');// do other stuffcatch(e){if(e.code=='ERR_INVALID_OPT_VALUE')console.error(e)&&process.exit(1)// because it's a programming errorelse ...;}
Maybe worth creating a new type? OptionError, or have all the codes include a substring?

jasnell · 2017-06-26T16:35:53Z

Over-generalization of error codes will make those fairly useless and users will end up parsing the error messages again to find out what actually happened. There's little harm in having specific error codes.

refack · 2017-06-26T16:43:35Z

Over-generalization of error codes will make those fairly useless and users will end up parsing the error messages again to find out what actually happened. There's little harm in having specific error codes.

I suggested two solutions: Either create an OptionError type, or use the a prefix ERR_INVALID_OPT_VALUE_XXX.
My main point is that these are predominantly programing errors and less often runtime errors.

refack · 2017-07-15T21:56:43Z

Superseded by #13976

jasnell · 2017-07-17T15:14:37Z

This is not fully superseded. I will refactor. Please do not close.

* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js

jasnell · 2017-07-18T00:41:30Z

Updated. I was waiting for #13976 to land so I could drop the internal/errors commit and refactor this. It should be good to go now.

refack

Would like to see obfuscated whiles replaced with for...

refack · 2017-07-18T14:39:09Z

- var i = 0;
- while (++i < byteLength && (mul *= 0x100))
- val += this[offset + i] * mul;
+var val = this[offset];


If we're churning let's get some butter: replace with for(...;...;...) to make it more readable. Performance should be on par

I'm going to leave these for now. We can hit those in a separate PR

refack · 2017-07-18T14:43:24Z

So I would like to see the obfuscated whiles replaced with for but that might be outside of this PR intended scope...

refack · 2017-07-18T14:46:52Z

Updated. I was waiting for #13976 to land so I could drop the internal/errors commit and refactor this. It should be good to go now.

@jasnell I'm assuming this is not in progress anymore? Also AFAICT it's semver-patch with the Error changes removed.

jasnell · 2017-07-24T03:49:28Z

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

jasnell · 2017-07-24T14:21:03Z

CI is green.

* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js PR-URL: #13807 Reviewed-By: Refael Ackermann <refack@gmail.com>

jasnell · 2017-07-24T14:29:53Z

Landed in 355523f

addaleax · 2017-07-24T17:17:22Z

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know and we’ll add the dont-land-on label.

jasnell · 2017-07-24T17:18:52Z

Unless it causes pain for backporting other things, this is not a priority to backport.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2017

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 20, 2017

jasnell force-pushed the refactor-buffer branch from 8da26a3 to 05db366Compare June 20, 2017 01:19

mscdex reviewed Jun 20, 2017
View reviewed changes

Comment threadlib/buffer.js Outdated
mscdexJun 20, 2017
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo

TimothyGu reviewed Jun 20, 2017
View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Jun 20, 2017

BridgeAR reviewed Jun 20, 2017
View reviewed changes

refack suggested changes Jun 23, 2017
View reviewed changes

refack closed this Jul 15, 2017

jasnell reopened this Jul 17, 2017

buffer: refactor module.exports, imports
426d42e
* Move to more efficient module.exports pattern * Refactor requires * Eliminate circular dependency on internal/buffer * Add names to some functions * Fix circular dependency error in assert.js

jasnell force-pushed the refactor-buffer branch from 05db366 to 426d42eCompare July 18, 2017 00:40

jasnell changed the title ~~buffer: refactor buffer exports, use internal/errors~~buffer: refactor buffer exportsJul 18, 2017

refack approved these changes Jul 18, 2017
View reviewed changes

refack removed the wip Issues and PRs that are still a work in progress. label Jul 18, 2017

jasnell removed semver-major PRs that contain breaking changes and should be released in the next major version. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jul 24, 2017

jasnell closed this Jul 24, 2017

addaleax added dont-land-on-v8.x and removed dont-land-on-v8.x labels Jul 24, 2017

addaleax added dont-land-on-v8.x and removed backport-requested-v8.x labels Jul 24, 2017

jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Uh oh!

Uh oh!

Conversation

jasnell commented Jun 20, 2017• edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 26, 2017

Uh oh!

refack commented Jun 26, 2017

Uh oh!

refack commented Jul 15, 2017

Uh oh!

jasnell commented Jul 17, 2017

Uh oh!

jasnell commented Jul 18, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 18, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

addaleax commented Jul 24, 2017

Uh oh!

jasnell commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jasnell commented Jun 20, 2017•
edited by refack
Loading

refack left a comment •
edited
Loading

refack commented Jul 18, 2017•
edited
Loading