buffer: implement buffer.atob and buffer.btoa by jasnell · Pull Request #37529 · nodejs/node · GitHub

Web Platform atob and btoa impl.

Just an initial get it working impl, not performance optimized.

Signed-off-by: James M Snell jasnell@gmail.com
Fixes: #3462

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

Kinda feeling "meh" on repeating the web's mistakes, but I can see the point for web compat.

I also don't really see the point for this API in Node.js, especially since ours (.toString on buffer) is arguably better and this API does not actually improve web compatibility by much since it's not global (and global probably isn't worth the breakage)

Yep. It's been a long standing open feature request, and is implemented in other platforms, it was a quick impl so I figured let's see what the reaction is. I'm fine with not having this either. I'll close this and close the feature request issue as a #wontfix.

I'm reopening this as I've had three separate, independent conversations with different folks in the past two weeks about the lack of atob/btoa in core. Yes, it's not as good as what we already have, and no, I don't see it as being critical, but the implementation is simple and easily maintained and it does improve cross-platform interop.

@jasnell why couldn’t they be globals? (I’m sure there’s discussion somewhere, but I’d love to read it linked from here)

Globals are semver major. If this lands I'll open a separate commit adding that. But this way it allows us the option of backporting the basic impls to 15 and 14

awesome, sounds like a great plan

Just making sure this doesn’t land until it has a big, bold warning against using it (or deprecation notice, tbh – I’d personally say that introducing this as a deprecated feature from the start would be just fine, but I’m guessing not everybody’s on the same page there…)

I'd be -1 for introducing as deprecated but I'm definitely adding a notice in the docs as suggested. I definitely say we need a new status indicator in the docs that Is-Not-Deprecated-But-Is-Also-Not-Recommended

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

The JS spec is going to use the term "legacy" for "things that are icky that we wish we could get rid of", if that helps :-)

That does help, actually. There are a few APIs in core that are currently deprecated but really fall under that legacy category instead. I may have to open a PR adding that in as an option

Signed-off-by: James M Snell <jasnell@gmail.com>

why do we always have to lazily load this constructor?

Yeah, it's gotten a bit out of hand really. I'm doing it for consistency. I recently landed a PR that updated the various AbortError instances to eliminate this in multiple places but there are still more. What would be good is a PR that moves DOMException into `internal/errors' and replaces these lazy loads for that.

I'm still -0 on this (since right now we're just adding the bad methods without improving compatibility since they're not global) but I see the merit if they're global in the next major.

lgtm

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Landed in 08770bc and c6855eb

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #37977 Notable changes: * buffer: * implement btoa and atob (James M Snell) #37529 * deps: * upgrade npm to 7.7.6 (Ruy Adorno) #37968 * doc: * add legacy status to stability index (James M Snell) #37784 * add @Linkgoron to collaborators (Nitzan Uziely) #37817 * http: * add http.ClientRequest.getRawHeaderNames() (simov) #37660

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 26, 2021

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 26, 2021

This comment has been minimized.

Sign in to view

addaleax reviewed Feb 26, 2021

View reviewed changes

Comment thread doc/api/buffer.md Outdated

Comment thread doc/api/buffer.md Outdated

Comment thread doc/api/buffer.md Outdated

Comment thread doc/api/buffer.md Outdated

addaleax reviewed Feb 26, 2021

View reviewed changes

Comment thread doc/api/buffer.md Outdated

jasnell closed this Feb 27, 2021

jasnell reopened this Mar 17, 2021

addaleax requested changes Mar 17, 2021

View reviewed changes

jasnell force-pushed the atob-btoa branch from 802710c to 32027d8 Compare March 17, 2021 18:21

jasnell force-pushed the atob-btoa branch from 32027d8 to f5b363a Compare March 17, 2021 18:27

jasnell requested a review from addaleax March 17, 2021 18:29

addaleax reviewed Mar 17, 2021

View reviewed changes

Comment thread doc/api/buffer.md Outdated

jasnell added 2 commits March 17, 2021 12:46

buffer: implement btoa and atob

ad44013

Signed-off-by: James M Snell <jasnell@gmail.com>

test: app atob web platform tests

12b20e5

Signed-off-by: James M Snell <jasnell@gmail.com>

jasnell force-pushed the atob-btoa branch from f5b363a to 12b20e5 Compare March 17, 2021 19:46

addaleax approved these changes Mar 17, 2021

View reviewed changes

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 17, 2021

targos reviewed Mar 17, 2021

View reviewed changes

jasnell mentioned this pull request Mar 17, 2021

buffer: expose btoa and atob as globals #37786

Closed

mcollina approved these changes Mar 18, 2021

View reviewed changes

jasnell added a commit that referenced this pull request Mar 18, 2021

buffer: implement btoa and atob

08770bc

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added a commit that referenced this pull request Mar 18, 2021

test: app atob web platform tests

c6855eb

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell closed this Mar 18, 2021

ruyadorno pushed a commit that referenced this pull request Mar 20, 2021

buffer: implement btoa and atob

dc9cd43

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ruyadorno pushed a commit that referenced this pull request Mar 20, 2021

test: app atob web platform tests

505f9c9

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ruyadorno mentioned this pull request Mar 30, 2021

v15.13.0 proposal #37977

Merged

targos pushed a commit that referenced this pull request May 1, 2021

buffer: implement btoa and atob

bf2d9f2

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

targos pushed a commit that referenced this pull request May 1, 2021

test: app atob web platform tests

5c60087

Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #37529 Fixes: #3462 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

tniessen mentioned this pull request May 14, 2023

doc: Don't think atob and btoa should be marked as legacy #40754

Closed

MichaelDeBoey mentioned this pull request Aug 26, 2023

feat(remix-node): use built-in atob & btoa remix-run/remix#7206

Merged

+              const lazyInvalidCharError = hideStackFrames((message, name) => {
+                if (DOMException === undefined)
+                  DOMException = internalBinding('messaging').DOMException;

buffer: implement buffer.atob and buffer.btoa by jasnell · Pull Request #37529 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer: implement buffer.atob and buffer.btoa - #37529

buffer: implement buffer.atob and buffer.btoa#37529jasnell wants to merge 2 commits intonodejs:masternodejs/node:masterfrom jasnell:atob-btoajasnell/node:atob-btoaCopy head branch name to clipboard

Conversation

jasnell commented Feb 26, 2021

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Feb 26, 2021

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjamingr commented Feb 27, 2021

Uh oh!

jasnell commented Feb 27, 2021

Uh oh!

jasnell commented Mar 17, 2021

Uh oh!

ljharb commented Mar 17, 2021

Uh oh!

jasnell commented Mar 17, 2021

Uh oh!

ljharb commented Mar 17, 2021

Uh oh!

addaleax 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!

jasnell commented Mar 17, 2021

Uh oh!

nodejs-github-bot commented Mar 17, 2021 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Mar 17, 2021

Uh oh!

jasnell commented Mar 17, 2021

Uh oh!

Uh oh!

targos Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Mar 18, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

buffer: implement buffer.atob and buffer.btoa#37529
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:atob-btoa

addaleax left a comment •
edited
Loading

nodejs-github-bot commented Mar 17, 2021 •
edited by jasnell
Loading