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

internet.jwt() fails on documentation website #3285

Open
9 of 10 tasks
xDivisionByZerox opened this issue Nov 26, 2024 · 17 comments · May be fixed by #3343
Open
9 of 10 tasks

internet.jwt() fails on documentation website #3285

xDivisionByZerox opened this issue Nov 26, 2024 · 17 comments · May be fixed by #3343
Assignees
Labels
c: bug Something isn't working c: docs Improvements or additions to documentation c: external Issues that related to external services p: 1-normal Nothing urgent
Milestone

Comments

@xDivisionByZerox
Copy link
Member

Pre-Checks

Describe the bug

When running faker.internet.jwt() on our documentation site, you receive an runtime error:

+esm:13 Uncaught TypeError: Unknown encoding: base64url
    at Uint8Array.Yi (+esm:13:5072)
    at Uint8Array.a [as toString] (+esm:13:5949)
    at ir (chunk-3QROWZCZ.js:1:5396)
    at kr.jwt (chunk-3QROWZCZ.js:1:18786)
    at <anonymous>:1:16

This might affect other browser environments as well (at least the ones using the same CDN).

Minimal reproduction code

Go to https://fakerjs.dev and open the dev tools.

Run the following:

await enableFaker();
faker.internet.jwt();

Additional Context

Our check for the existance of Buffer fails because the file (https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/+esm) loaded from the cdn includes a Buffer polyfill:

image

Environment Info

Tested in Microsoft Edge - Version 131.0.2903.63 (Offizielles Build) (64-Bit)

Which module system do you use?

  • CJS
  • ESM

Used Package Manager

npm

@xDivisionByZerox xDivisionByZerox added the c: bug Something isn't working label Nov 26, 2024
@xDivisionByZerox
Copy link
Member Author

Would be glad if someone can confirm this bug.

@sounmind
Copy link

Same with me.

image

Environment Info

Mac, Chrome: 131.0.6778.69 (Official Build) (arm64)

@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

I can confirm the error as well on Firefox For Windows.
But I'm unable to explain it.

@ST-DDT ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent labels Nov 26, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

Looks like it's getting poly filled:

https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/+esm

  • Search for Buffer (case sensitive+whole word)
/*!
 * The buffer module from node.js, for the browser.
 *
 * @author   Feross Aboukhadijeh <[email protected]> <http://feross.org>
 * @license  MIT
 */

https://github.com/feross/buffer

@xDivisionByZerox
Copy link
Member Author

Looks like it's getting poly filled:

cdn.jsdelivr.net/npm/@faker-js/[email protected]/+esm
[...]

I already came to the same conclusion in the "Additional Context" section of the issue. Still, I'm unable to grasp why this happens, since we have zero runtime dependencies..

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Nov 26, 2024

Might the bundling from JSDelivery cause trouble? You can see at the top of the +esm file:

/**
 * Bundled by jsDelivr using Rollup v2.79.1 and Terser v5.19.2.
 * Original file: /npm/@faker-js/[email protected]/dist/index.js
 *
 * Do NOT use SRI with dynamically generated files! More information: https://www.jsdelivr.com/using-sri-with-dynamic-files
 */
[...]

I tried the mentioned original file. When I do const { faker } = await import('https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/dist/index.js') our documentation site I can use faker without problems.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

I already came to the same conclusion in the "Additional Context" section of the issue. Still, I'm unable to grasp why this happens, since we have zero runtime dependencies..

Oh, I haven't seen that part.

I tried the mentioned original file. When I do const { faker } = await import('https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/dist/index.js') our documentation site I can use faker without problems.

Should we replace our import references?

@xDivisionByZerox
Copy link
Member Author

Should we replace our import references?

I guess that would be the best to not rely on downstream implementations. 🤔

@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

I guess that would be the best to not rely on downstream implementations. 🤔

Yeah. especially since it seems to be stuck for many months now.

@xDivisionByZerox xDivisionByZerox self-assigned this Nov 26, 2024
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Nov 26, 2024

I checked again. The advantage of the +esm import is that the original file import fires 78 requests to retrieve all the chunks to load faker. This is about 50% slower from my quick testing. Can you run some tests on your end as well? Don't forget to disable the browser cache in the dev tools tho.

@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

console.time("bundled")
await import("https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/+esm");
console.timeEnd("bundled")
console.time("original")
await import("https://cdn.jsdelivr.net/npm/@faker-js/[email protected]/dist/index.js");
console.timeEnd("original")

Mostly:

bundled: 710ms
original: 976ms

Sometimes:

bundled: 204ms
original: 413ms

@ST-DDT
Copy link
Member

ST-DDT commented Nov 26, 2024

We could push a single file build onto our website and use that instead of an external source (docs/public/faker.js).
That way we could even try new features from PRs in the previews.

Might be useful in combination with:

@ST-DDT ST-DDT added this to the vAnytime milestone Nov 26, 2024
@xDivisionByZerox xDivisionByZerox removed their assignment Nov 27, 2024
@xDivisionByZerox
Copy link
Member Author

We could push a single file build onto our website and use that instead of an external source (docs/public/faker.js).
That way we could even try new features from PRs in the previews.

Lets discuss this issue on thursdays team meeting. Since this issue is only present on the website if you are using functions that call the internal base64-helpers, the scope of this bug is pretty limited. If you think this is more urgend, feel free to provide a PR with your suggestion of a fix.

@matthewmayer
Copy link
Contributor

matthewmayer commented Nov 27, 2024

looks like there may be a way to stop jsdelivr adding the Buffer polyfill:
jsdelivr/jsdelivr#18263 (comment)

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Nov 27, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Nov 27, 2024

Not urgent as no users have reported this error yet.

This might also affect users that bundle faker into production code for whatever reason and applying the partial polyfil. So the neat trick with node:buffer=false might be worth applying anyway.

I consider the browser self bundling a separate feature.

@xDivisionByZerox xDivisionByZerox added the c: external Issues that related to external services label Nov 27, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Nov 28, 2024

Team Decision

  • We will change our toBase64Url method, to always use toBase64().replaceAll() with a TODO comment pointing to the upstream issue

@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Nov 28, 2024
@ST-DDT ST-DDT linked a pull request Dec 28, 2024 that will close this issue
@ST-DDT ST-DDT linked a pull request Dec 28, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working c: docs Improvements or additions to documentation c: external Issues that related to external services p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants