-
-
Notifications
You must be signed in to change notification settings - Fork 949
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
fix: test before using Buffers #3400
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #3400 +/- ##
==========================================
- Coverage 99.97% 99.97% -0.01%
==========================================
Files 2811 2811
Lines 216965 216972 +7
Branches 941 941
==========================================
+ Hits 216914 216919 +5
- Misses 51 53 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly fine with both solutions. Having the explicit check function with documentation linking to the related upstream issues, might remind someone in the future to look into this and remove the workaround. On the other side, most of us are propably suvscribed to the external issues as well and will get notified anyway.
My opinionated solution: /* eslint-disable no-undef -- This file serves as a compatibility layer between environments */
/**
* Checks whether the environment supports the given encoding on the `Buffer` class.
* This is required because some `Buffer` polyfills do not support all encodings.
*
* @param encoding The encoding to check.
*
* @see https://www.npmjs.com/package/buffer
* @see https://github.com/feross/buffer/issues/309
*/
function bufferFeatureCheck(encoding: BufferEncoding): boolean {
try {
return typeof Buffer.from("test").toString(encoding) === "string";
} catch {
return false;
}
}
const SUPPORTS_BASE64_BUFFER =
typeof Buffer === "undefined" || !bufferFeatureCheck("base64");
const SUPPORTS_BASE64_URL_BUFFER =
typeof Buffer === "undefined" || !bufferFeatureCheck("base64url");
/**
* This works the same as `Buffer.from(input).toString('base64')`
* to work on both Node.js and browser environment.
*
* @internal
*
* @param input The string to encode to Base64.
*
* @returns Base64 encoded string.
*
* @see https://datatracker.ietf.org/doc/html/rfc4648
*
* @example const encodedHeader = toBase64(JSON.stringify(header));
*/
export function toBase64(input: string): string {
if (SUPPORTS_BASE64_BUFFER) {
return Buffer.from(input).toString("base64");
}
const utf8Bytes = new TextEncoder().encode(input);
const binaryString = Array.from(utf8Bytes, (byte) =>
String.fromCodePoint(byte)
).join("");
return btoa(binaryString);
}
/**
* This works the same as `Buffer.from(input).toString('base64url')`
* to work on both Node.js and browser environment.
*
* @internal
*
* @param input The string to encode to Base64 URL.
*
* @returns Base64 URL encoded string.
*
* @see https://datatracker.ietf.org/doc/html/rfc4648
*
* @example const encodedHeader = toBase64Url(JSON.stringify(header));
*/
export function toBase64Url(input: string): string {
if (SUPPORTS_BASE64_URL_BUFFER) {
return Buffer.from(input).toString("base64url");
}
return toBase64(input)
.replaceAll("+", "-")
.replaceAll("/", "_")
.replaceAll(/=+$/g, "");
} I know this moves the check into the function, but makes it (to me) much more readable. I don't think that the condition has a to bad performance compared to actually craft the Edit: we might even be able to remove the |
Fixes #3285
Adds a feature check before committing to use the Buffer methods.
Alternative
Remove the Buffer "shortcut" check and usage from the base64url method.