-
Notifications
You must be signed in to change notification settings - Fork 58
Rn/prebuild mdx transforms #910
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
Vercel Previews Deployed
|
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This reverts commit 6beea78.
"@utils/*": ["app/utils/*"], | ||
"@api/*": ["app/api/*"] | ||
} | ||
"#utils/*": ["./app/utils/*"], |
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.
These really should not be needed, but during next.js build them seem to ignore Node import path aliases... 😢
|
||
if (binaryExists) { | ||
try { | ||
if (existsSync(filename)) { |
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.
Yup we delete the old binary and unzip a new binary each time. It's super duper quick and it means that we don't have to worry about folks having old binaries running. Which is a big win.
import { batchPromises } from '../utils/batch-promises.mjs' | ||
import { readFile, writeFile } from 'node:fs/promises' | ||
import { listFiles } from '#scriptUtils/list-files.mjs' | ||
import { batchPromises } from '#scriptUtils/batch-promises.mjs' |
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.
Were the path name changes necessary because of a change for this update? If not, they are out of scope for this PR and should have been a separate PR.
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.
LGTM! Ran this all locally and through docker and it sped up the build time quite a bit for me. I think it would be a good idea for someone with a windows computer to test this just to be sure it works for other operating systems.
@RubenSandwich Please benchmark the time differences locally and in deployments before & after. We are trying to track this closely as new products are added and for improvements to the process. |
Description
Speeds up prebuild by running a prebuilt prebuild binary compiled with bun.
This did require a few changes:
Other things are where not strictly necessary but where done to improve the code/make writing the check-prebuild-binaries.yml GHA easier:
/prebuild
folderimport fs from 'fs'
import fs from 'node:fs'
productConfig.mjs
to the root folder as it's used by both /app and /scriptsBenchmarks
BEFORE (on main):
AFTER (on this branch):
Change in execution time: 69.104s / 139.261s = 0.4962
So with these changes
prebuild
is ~0.50% faster and saves ~1 min 10s per run.Testing