Skip to content

Conversation

@ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Nov 8, 2018

The encodeStr function and qsEscape function are almost the same.
Extract encodeStr function and move it to internal for reusable.

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

cc @mscdex

@nodejs-github-bot nodejs-github-bot added the querystring Issues and PRs related to the built-in querystring module. label Nov 8, 2018
@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 8, 2018

Is the Benchmark CI result acceptable ? 🤔

BTW, didn't understand why we use function

var encode = QueryString.escape;

instead of

var encode = qsEscape;

@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

BTW, didn't understand why we use function

var encode = QueryString.escape;

instead of

var encode = qsEscape;

The reason is that we allow users to override the escape function.

@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

Is the Benchmark CI result acceptable ?

I suppose so.

@Trott
Copy link
Member

Trott commented Nov 16, 2018

@nodejs/url This doesn't technically need another review, but it would be nice.

@Trott
Copy link
Member

Trott commented Nov 21, 2018

Landed in db9a745

@Trott Trott closed this Nov 21, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 21, 2018
PR-URL: nodejs#24242 Reviewed-By: James M Snell <jasnell@gmail.com>
@ZYSzys ZYSzys deleted the encodeStr branch November 21, 2018 02:57
targos pushed a commit that referenced this pull request Nov 21, 2018
PR-URL: #24242 Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #24242 Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
4 tasks
codebytere pushed a commit that referenced this pull request Jan 13, 2019
PR-URL: #24242 Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24242 Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jan 15, 2019
codebytere pushed a commit that referenced this pull request Jan 29, 2019
PR-URL: #24242 Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

querystring Issues and PRs related to the built-in querystring module.

6 participants