FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v14.x backport] buffer: add base64url encoding option by targos · Pull Request #39702 · nodejs/node · GitHub

/ node Public

[v14.x backport] buffer: add base64url encoding option - #39702

Closed
targos wants to merge 2 commits into
nodejs:v14.x-stagingfrom
targos:base64-url-v14
ConversationCommits (2)ChecksFiles changed
Closed

[v14.x backport] buffer: add base64url encoding option#39702
targos wants to merge 2 commits into
nodejs:v14.x-stagingfrom
targos:base64-url-v14

Conversation

targos commented Aug 8, 2021

Copy link
Copy Markdown
Member

The fixup commit is a partial application of the changes from dae283d that are needed for this to work.

github-actions Bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v14.x labels Aug 8, 2021

targos commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

/cc @panva @jasnell @addaleax

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2021

nodejs-github-bot commented Aug 8, 2021

Copy link
Copy Markdown
Collaborator

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

panva commented Aug 8, 2021

Copy link
Copy Markdown
Member

There was also this doc fix fd02dac you could pick

targos commented Aug 8, 2021

Copy link
Copy Markdown
Member Author

fd02dac is already in v14.x

nodejs-github-bot commented Aug 8, 2021

Copy link
Copy Markdown
Collaborator

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

targos commented Aug 12, 2021
View reviewed changes
Comment thread src/node.h Outdated
Comment on lines 667 to 678

targos Aug 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@addaleax IIUC, this is OK ABI-wise because the new element doesn't change the other values of the enum? But ParseEncoding can now return this new value and programs that don't expect it would break? Can we do something about it (other than not backporting the feature)?

addaleax Aug 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@targos Yes, exactly, it’s ABI-compatible but not API-compatible, strictly speaking, and I don’t think we could really do something about it other than modifying ParseEncoding() to not return that value when called from userland code.

The problem with that is that, since we fall back to a default encoding in that case, ParseEncoding(isolate, "base64url") == LATIN1, which would seem like a bug.

I’d probably just leave this as-is, it’s unlikely to cause problems for a lot of addons (and even then only when somebody actually uses base64url with it).

panva and others added 2 commits August 13, 2021 10:24
buffer: add base64url encoding option
0a9ef60 
PR-URL: nodejs#36952
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
fixup! buffer: add base64url encoding option
8450037 
Backport parts of nodejs@dae283d
targos pushed a commit that referenced this pull request Aug 13, 2021
buffer: add base64url encoding option
a343956 
Backport parts of dae283d

PR-URL: #36952
Backport-PR-URL: #39702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

targos commented Aug 13, 2021

Copy link
Copy Markdown
Member Author

Landed in a343956

targos closed this Aug 13, 2021
targos deleted the base64-url-v14 branch August 13, 2021 15:48
MylesBorins pushed a commit that referenced this pull request Aug 31, 2021
buffer: add base64url encoding option
223494c 
Backport parts of dae283d

PR-URL: #36952
Backport-PR-URL: #39702
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
panva panva approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL