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

docs: use embedded build for preview examples #3343

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Dec 28, 2024

Fixes #3342, #3301 (comment)


This bundles the project into a single file and makes it available to the browser console for next and PRs.
To keep traffic low on our website and help counting the usage of the versions, the release sites keep using the jsdelivr CDN links.

You can test it using the new language method.

grafik

grafik
grafik

@ST-DDT ST-DDT added c: docs Improvements or additions to documentation p: 1-normal Nothing urgent labels Dec 28, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Dec 28, 2024
@ST-DDT ST-DDT requested review from a team December 28, 2024 13:58
@ST-DDT ST-DDT self-assigned this Dec 28, 2024
Copy link

netlify bot commented Dec 28, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 14aac9c
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/67755863eb8afb0008cc173a
😎 Deploy Preview https://deploy-preview-3343.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Dec 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (ece4f16) to head (14aac9c).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3343   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files        2811     2811           
  Lines      217023   217023           
  Branches      942      941    -1     
=======================================
  Hits       216972   216972           
  Misses         51       51           

@@ -10,10 +10,11 @@
"generate": "run-s generate:locales generate:api-docs",
"generate:api-docs": "tsx ./scripts/apidocs.ts",
"generate:locales": "tsx ./scripts/generate-locales.ts",
"docs:build": "run-s generate:api-docs docs:build:run",
"docs:build": "run-s generate:api-docs docs:build:embedded docs:build:run",
"docs:build:embedded": "tsup-node --entry.faker src/index.ts --format esm --outDir docs/public --no-dts --no-splitting --no-clean",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is embedded the right term here? Any other suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seams correct enought. Since this script manipulates the origin the faker lib is loaded from you could also embrace the web standart: "docs:build:same-origin" or "docs:build:no-cors". I don't thinkk it matters to much tho.

@@ -14,4 +14,7 @@ export default defineConfig({
minify: true,
sourcemap: false,
splitting: true,
esbuildOptions(options) {
options.charset = 'utf8'; // Prevent Unicode escaping
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reduces the size from the (docs) shipped faker.js file from 20MB to 2.7MB (due to not using unicode escape sequences!? Not sure whether there are other things at play as well).
Does that break anything for our users (in our normal builds)?
Or is there a better way to do this?

For comparison:

Branch /dist /docs/public/faker.js
next 8.6 MB 20 MB
this 6.6 MB 2.7 MB

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we make this change and see if anyone complains? I currently do not see how this would affect our users. Maybe do this change separatly? That way its not hidden in a seemingly unrelated PR?

@matthewmayer
Copy link
Contributor

This appears to fix jwt() too - although I'm not sure why it fails as that was added in 9.1.0... https://next.fakerjs.dev/api/internet.html#jwt

image

@ST-DDT
Copy link
Member Author

ST-DDT commented Dec 28, 2024

Thats caused by the bad Buffer polyfil. Not sure where we discussed that.

EDIT:

@ST-DDT ST-DDT linked an issue Dec 28, 2024 that may be closed by this pull request
10 tasks
This was referenced Jan 2, 2025
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extend the [CONTRIBUTING page] with the new script as well?

@@ -10,10 +10,11 @@
"generate": "run-s generate:locales generate:api-docs",
"generate:api-docs": "tsx ./scripts/apidocs.ts",
"generate:locales": "tsx ./scripts/generate-locales.ts",
"docs:build": "run-s generate:api-docs docs:build:run",
"docs:build": "run-s generate:api-docs docs:build:embedded docs:build:run",
"docs:build:embedded": "tsup-node --entry.faker src/index.ts --format esm --outDir docs/public --no-dts --no-splitting --no-clean",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seams correct enought. Since this script manipulates the origin the faker lib is loaded from you could also embrace the web standart: "docs:build:same-origin" or "docs:build:no-cors". I don't thinkk it matters to much tho.

@@ -14,4 +14,7 @@ export default defineConfig({
minify: true,
sourcemap: false,
splitting: true,
esbuildOptions(options) {
options.charset = 'utf8'; // Prevent Unicode escaping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we make this change and see if anyone complains? I currently do not see how this would affect our users. Maybe do this change separatly? That way its not hidden in a seemingly unrelated PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: docs Improvements or additions to documentation p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh examples button doesn't work for unreleased methods internet.jwt() fails on documentation website
3 participants