Skip to content
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.

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

feat: Node v6.10 API compatibility #2

Merged
merged 24 commits into from
Apr 6, 2017
Merged

feat: Node v6.10 API compatibility #2

merged 24 commits into from
Apr 6, 2017

Conversation

SpainTrain
Copy link
Owner

@SpainTrain SpainTrain commented Apr 6, 2017

BREAKING CHANGE: This is a complete rewrite based upon the actual node source code and tests.


This change is Reviewable

@pcgilday
Copy link

pcgilday commented Apr 6, 2017

Reviewed 14 of 22 files at r1, 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


a discussion (no related file):
I think it looks good, been a while since I've reviewed some of this syntax though haha 😄

Once builds are passing I'll give a final look and squirrel.


src/index.js, line 90 at r2 (raw file):

  var n, m, hexchar, c;

  for (var inIndex = 0, outIndex = 0; ; inIndex++) {

I've always preferred the format ++inIndex, which is used below. Either way I would suggest to stick to one.


src/index.js, line 222 at r2 (raw file):

      continue;
    }
    // Surrogate pair

BTW - TIL what a surrogate pair was 🙃


Comments from Reviewable

@SpainTrain
Copy link
Owner Author

Reviewed 16 of 22 files at r1.
Review status: 19 of 22 files reviewed at latest revision, 2 unresolved discussions.


src/index.js, line 90 at r2 (raw file):

Previously, pcgilday (Patrick Gilday) wrote…

I've always preferred the format ++inIndex, which is used below. Either way I would suggest to stick to one.

as indicated in the comment at the top of this file, this is copied from node source https://github.com/nodejs/node/blob/v6.10.2/lib/querystring.js

plus this isn't just a matter of style/taste, prefix is a different operation than postfix https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Arithmetic_Operators#Increment_()


Comments from Reviewable

@SpainTrain
Copy link
Owner Author

Reviewed 2 of 22 files at r1, 6 of 8 files at r2, 1 of 3 files at r3, 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@SpainTrain SpainTrain merged commit 68fdc79 into master Apr 6, 2017
@SpainTrain SpainTrain deleted the testing-etc branch April 6, 2017 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants