Add support for encoding URL-safe base64 #53
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -50,11 +53,12 @@ function _byteLength (b64, validLen, placeHoldersLen) { | |||
| return ((validLen + placeHoldersLen) * 3 / 4) - placeHoldersLen | |||
| } | |||
|
|
|||
| function toByteArray (b64) { | |||
| function toByteArray (b64, alphabet) { | |||
There was a problem hiding this comment.
Why does decoding need to take an alphabet? You set revLookups.url = revLookups._ anyway, so this seems like it adds nothing. Am I missing something?
There was a problem hiding this comment.
i felt like keeping the external api symmetrical was valuable, i think i prefer it this way but happy to change if you like.
There was a problem hiding this comment.
I see. I agree that symmetry is nice, but I think that's outweighed by having a parameter that has no effect. Can you change it?
There was a problem hiding this comment.
ok. how much do you want to remove? the entire revlookups table? as in, go back to having a single variable? if anyone wants to experiment with or add additional alphabets later it will be much more difficult...
There was a problem hiding this comment.
basically, now that i'm looking at it, i can see it's not just the interface, it's the implementation as well. the parameter doesn't have "no effect", it's just that the default alphabet happens to include a hack (for node compatibility) to allow decoding url safe base 64, so we're reusing the default reverse lookup for both alphabets to save a little memory.
so yea we could remove just the parameter from the interface, but then why bother keeping the symmetrical implementation?
There was a problem hiding this comment.
it also seems like it might be worth at least encouraging user code to be specific about what is being decoded if possible - i'm thinking that reading b64.toByteArray(str, 'url') in my source might save me some confusion down the road
There was a problem hiding this comment.
and just to really beat it to death... from https://tools.ietf.org/html/rfc4648#section-5:
This encoding may be referred to as "base64url". This encoding
should not be regarded as the same as the "base64" encoding and
should not be referred to as only "base64". Unless clarified
otherwise, "base64" refers to the base 64 in the previous section.
|
Any additional thoughts on merging this? Before I reinvent the wheel, I'd rather switch to your existing implementation. |
|
oops, had forgotten about this. I personally still like having the (totally optional!) alphabet arg, but Feross is right that it should "do something". I just pushed a few commits to try and clear up the difference between real base64url support and the node compat hack where the default alphabet also decodes base64url, see what you think. |
Using string replaceAll since base64 is external base64-js. Dependency does not yet support base64url beatgammit/base64-js#53
Using string replaceAll since base64 is external base64-js. Dependency does not yet support base64url beatgammit/base64-js#53
|
I implemented an extra optional |
Using string replaceAll since base64 is external base64-js. Dependency does not yet support base64url beatgammit/base64-js#53
|
Hi @jessetane, thanks for the pull request. We have moved the repository, if you are still interested in making a contribution please open a new pull request there. |
There are a bunch of modules on npm that do this already, but it seems like most of them depend on another module (sometimes buffer itself) and then add regex replace or similar, or include lots of features I don't want. Since we already do decoding here, I figure adding encoding just makes things symmetrical and so doesn't feel too out of place.
Note that performance remains the same with this approach but there is a tiny memory penalty (the extra alphabet itself should only be another ~128 bytes?). Ideas for other approaches or references to alternate modules welcome.