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

btoa() and atob() #3462

Closed
silverwind opened this issue Oct 21, 2015 · 57 comments
Closed

btoa() and atob() #3462

silverwind opened this issue Oct 21, 2015 · 57 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@silverwind
Copy link
Contributor

All major browsers expose the btoa and atob globals for encoding ASCII strings as base64 and vice versa. It'd be beneficial for the "isomorphic" javascript topic if we'd provide these too:

As for the implementation, we can't Buffer('string').toString('base64') because that would encode unicode while atob and btoa must per spec throw on charcodes > 255. https://github.com/mathiasbynens/base64 looks like a solid implemenation we could pretty much drop in.

@silverwind silverwind added the feature request Issues that request new features to be added to Node.js. label Oct 21, 2015
@MylesBorins
Copy link
Contributor

I'd be up for writing a bunch of tests if people want to see this happen

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

While I understand the desire on this, it could just as easily be implemented as a userland module that registers the globals. I'm -0 on adding it in to core.

@mscdex
Copy link
Contributor

mscdex commented Oct 21, 2015

I think I'm -1 on this for the same reason.

@trevnorris
Copy link
Contributor

function btoa(str) {
  if (Buffer.byteLength(str) !== str.length)
    throw new Error('bad string!');
  return Buffer(str, 'binary').toString('base64');
}

Think that will about cover it.

Update: actually, that's a bad length check and I'm too tired to work the correct one. we'd probably have to expose v8::String::ContainsOnlyOneByte() for the fastest option.

@Fishrock123
Copy link
Member

I'd rather not add more globals, ever if possible.

@tflanagan
Copy link
Contributor

The fact that you are linking to a userland package that already gets over a 1000 downloads a week (via npm) is solid evidence that the need for this, in core at least, is very minimal

@silverwind
Copy link
Contributor Author

By the way: Why doesn't v8 provide functions like these?

@bnoordhuis
Copy link
Member

They're browser extensions, they're not part of the ECMAScript spec: https://html.spec.whatwg.org/multipage/webappapis.html#atob

@jasnell
Copy link
Member

jasnell commented Oct 21, 2015

Feel free to continue discussing, but given the -1's, I'm closing.

@jasnell jasnell closed this as completed Oct 21, 2015
henvic added a commit to henvic/api.js that referenced this issue Nov 30, 2015
henvic added a commit to henvic/api.js that referenced this issue Nov 30, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 1, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 15, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 22, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 22, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 22, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 22, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 22, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 23, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 24, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 24, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 24, 2015
henvic added a commit to henvic/api.js that referenced this issue Dec 24, 2015
henvic added a commit to henvic/api.js that referenced this issue Jan 5, 2016
@buzinas
Copy link

buzinas commented Apr 7, 2016

The fact that you are linking to a userland package that already gets over a 1000 downloads a week (via npm) is solid evidence that the need for this, in core at least, is very minimal

In fact, that's because everyone uses the packages atob and btoa, e.g:
https://www.npmjs.com/package/atob

~500k downloads per month

@kav
Copy link

kav commented Jul 11, 2016

The fact that you are linking to a userland package that already gets over a 1000 downloads a week (via npm) is solid evidence that the need for this, in core at least, is very minimal

Regarding the above. Isn't the fact that thousands of users are using a userland package for this a reason to include it rather than the reverse?

Perhaps I'm confused but if the userland package had very few downloads wouldn't you find yourself saying; "well no one really seems to need this". I know I would.

A bit lost on the reasoning.

@addaleax
Copy link
Member

The reasoning here is that the situation seems quite well for everyone without something being provided by Node core.

@kav
Copy link

kav commented Jul 11, 2016

Yes but isn't the existance of a rarely used userland package also a valid reason to say it shouldn't go into core? So if either a userland package is used a lot (per your comment) or a little (per mine) it shouldn't be in node core. How then does that have any bearing on the question at all?

@bmeck bmeck reopened this Oct 6, 2020
@bmeck
Copy link
Member

bmeck commented Oct 6, 2020

looking at WHAT spec btoa/atob only handle strings with 8bits, so emoji etc. will throw errors, likely those APIs are not sufficient, but I do not see a web API that does universal base64 transformation except FileReader.prototype.readAsDataURL.

@MylesBorins
Copy link
Contributor

for base 64 there is https://en.wikipedia.org/wiki/Base64#RFC_4648

I have helped to maintain a library that has quite a bit of usage in this space

https://github.com/brianloveswords/base64url with 877k weekly downloads.

Could be worth looking at that for API inspiration

jasnell added a commit to jasnell/node that referenced this issue Feb 26, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: nodejs#3462
@jasnell
Copy link
Member

jasnell commented Feb 27, 2021

Opened a pr that implemented this and the immediate reaction was meh given that our current mechanisms for this are better and it's something that is easily handled by userland. Closing the issue as it's not something we're likely to land.

@jasnell jasnell closed this as completed Feb 27, 2021
jasnell added a commit that referenced this issue Mar 18, 2021
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 issue Mar 18, 2021
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 issue Mar 20, 2021
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 issue Mar 20, 2021
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>
@devinivy
Copy link

In nodejs/citgm#852 (comment) it was flagged that there are some new globals in node v16 including atob() and btoa() that our test suites should account for. If these are here to stay we'll ensure our test suites allow these globals, but based on what @jasnell mentioned above it seems like they may not land in node v16. Any clarity on this would be much appreciated!

@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2021

@devinivy these landed already, so I think it's safe for lab to start tracking them as allowed globals.

targos pushed a commit that referenced this issue May 1, 2021
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 issue May 1, 2021
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>
@Tofandel
Copy link

Tofandel commented Jun 19, 2021

It's weird, so most people were against it but it landed anyway under the radar..

@jasnell
Copy link
Member

jasnell commented Jun 19, 2021

@Tofandel ... there is no "under the radar" here. All changes go through the same review and approval process before landing, and all discussions/reviews are public.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.