-
Notifications
You must be signed in to change notification settings - Fork 29.2k
docs: fix TS error in Node.js runtime local assets example #82672
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
Conversation
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Right, I tried to address several months ago haha #75628 - let me do a memory refresh |
Hi @icyJoseph just following up, whenever you get a chance, could you please review this PR? I saw your earlier comment about #75628, so I think you’re familiar with the context. Thanks a lot for your time! |
Since this is my first contribution to Next.js, I’m not fully sure about the next steps, how PRs typically get reviewed, and whether I should tag someone specific or just wait for it to get picked up. Could you please guide me on the best way to proceed and how reviews are usually requested in this repo? Thanks a lot! |
So I kind of disagree with this PR, at least in its current form. The problem here is because:
Satori is the engine used under the hood. The idea with supporting ArrayBuffer is a DX advantage. The way I see it, we should perhaps combine this PR and my PR, to show explicitly a use case using base64 that doesn't need any TypeScript handling, and my PR that shows ArrayBuffer, with a Also, I think your PR can be shortened with: const logoData = await readFile(join(process.cwd(), 'logo.png'), 'base64')
const logoSrc = `data:image/png;base64,${logoData}` Something like that ~ Do you mind cherry-picking or re-applying the PR I linked above, and have the two snippets? Let's see how it looks like. |
Hi @icyJoseph sure will add both 😊 |
Hey @icyJoseph, |
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
docs/01-app/03-api-reference/03-file-conventions/01-metadata/opengraph-image.mdx
Outdated
Show resolved
Hide resolved
…pengraph-image.mdx Co-authored-by: Joseph <[email protected]>
What?
Updated the documentation for
opengraph-image
/twitter-image
to fix the Node.js runtime with local assets example.Why?
The existing snippet did not type-check cleanly with Next.js 15+ and could confuse developers.
This update improves developer experience by ensuring the snippet is immediately usable and accurate.
How?
readFile
+ Base64 encoding instead of invalid file URLs.Checklist
pnpm prettier-fix
pnpm build && pnpm lint
)Fixes #75583 & #75625 (if considered same class of issue)