Don't inline all env vars in Standalone CLI #19391
Conversation
This is equivalent to the old build setup but doesn’t need to use CLI commands to build the binaries
Right now `NODE_PATH` gets inlined into the build causing linux binaries to additionally search for `/Users/runner/…` directories
Async APIs became capable of reading embedded files in Bun v1.2.3
WalkthroughRemoves the synchronous file-read fallback Pre-merge checks✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/@tailwindcss-node/src/compile.ts(0 hunks)packages/@tailwindcss-standalone/package.json(1 hunks)packages/@tailwindcss-standalone/scripts/build.ts(1 hunks)packages/@tailwindcss-standalone/src/index.ts(0 hunks)playgrounds/vite/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/@tailwindcss-node/src/compile.ts
- packages/@tailwindcss-standalone/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: macOS / oxide
- GitHub Check: macOS / postcss
- GitHub Check: macOS / webpack
- GitHub Check: macOS / upgrade
- GitHub Check: macOS / vite
- GitHub Check: Windows / webpack
- GitHub Check: Windows / oxide
- GitHub Check: Windows / postcss
- GitHub Check: Windows / cli
- GitHub Check: Windows / upgrade
- GitHub Check: Windows / vite
- GitHub Check: Windows
🔇 Additional comments (6)
playgrounds/vite/package.json (1)
21-21: LGTM! Consistent version alignment.The Bun version bump to ^1.3.3 aligns with the same update in the standalone package. Assuming the version verification from the standalone package confirms validity, this change maintains consistency across the repository.
packages/@tailwindcss-standalone/scripts/build.ts (5)
14-22: LGTM! Comprehensive platform coverage.The build target array covers all major platforms with appropriate baseline targets for x64 to ensure broader hardware compatibility. The structure is clear and maintainable.
41-54: Excellent! This fix directly addresses issue #19389.The
definesection now selectively controls environment variables at build time, preventing platform-specific paths from being inlined. Key improvements:
NODE_PATHis explicitly set to empty string, preventing macOS-specific paths from being embedded in binaries for other platforms.PLATFORM_LIBCis dynamically determined per target, ensuring correct runtime behavior.NAPI_RS_*variables are controlled to reduce bundle size and prevent unnecessary WASI builds.This approach ensures each platform binary contains only appropriate paths for its target OS, eliminating the cross-platform filesystem lookup issues described in #19389.
56-65: LGTM! Appropriate compile configuration.Disabling
autoloadDotenvandautoloadBunfigis correct for a standalone CLI, ensuring consistent behavior across different user environments. The target and outfile settings are properly configured.
67-76: LGTM! Plugin correctly excludes WASI build.The plugin properly prevents the WASI build from being bundled by returning empty contents for
tailwindcss-oxide.wasi.cjs. This, combined with theNAPI_RS_FORCE_WASIdefine, ensures WASI code is not included in the standalone binaries, reducing size and complexity.
8-10: The USERPROFILE workaround is properly isolated to this build script and doesn't impact other code paths. Since no other files reference this variable, setting it to an empty string is a safe, targeted fix for the Bun binary download issue on Windows CI.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/@tailwindcss-standalone/scripts/build.ts (1)
97-97: Checksum format is now correct.The double-prefix issue flagged in the previous review has been resolved. The current code correctly uses
./${name}instead of./tailwindcss-${name}, producing proper checksum entries like<hash> ./tailwindcss-linux-arm64.
🧹 Nitpick comments (1)
packages/@tailwindcss-standalone/scripts/build.ts (1)
81-82: Consider checking build success status.The entry-point validation is good, but you could optionally check
result.successor inspectresult.logsto catch build warnings or failures earlier with more context. The current approach relies on Bun.build throwing on failure, which may be sufficient.let entry = result.outputs.find((output) => output.kind === 'entry-point') - if (!entry) throw new Error(`Build failed for ${target}`) + if (!entry || !result.success) { + console.error(result.logs) + throw new Error(`Build failed for ${target}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@tailwindcss-standalone/scripts/build.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Linux / cli
- GitHub Check: macOS / oxide
- GitHub Check: macOS / webpack
- GitHub Check: Linux / postcss
- GitHub Check: macOS / vite
- GitHub Check: Linux / webpack
- GitHub Check: macOS / upgrade
- GitHub Check: Linux / upgrade
- GitHub Check: Linux / vite
- GitHub Check: Windows / oxide
- GitHub Check: Windows / cli
- GitHub Check: Windows / webpack
- GitHub Check: Windows / upgrade
- GitHub Check: Windows / postcss
- GitHub Check: Windows / vite
🔇 Additional comments (4)
packages/@tailwindcss-standalone/scripts/build.ts (4)
8-10: LGTM—documented CI workaround.The USERPROFILE clearing is intentional for Windows CI compatibility with Bun binary downloads. The comment clearly documents the rationale.
14-24: Build matrix covers all platforms appropriately.The target definitions are comprehensive and well-structured. The TypeScript suppression at lines 17-18 is acceptable given the documented type/runtime mismatch.
33-42: Build configuration is appropriate for standalone binaries.The Bun.build configuration correctly:
- Targets Node compatibility
- Preserves names for better debugging
- Disables .env and bunfig loading (per PR objectives to prevent environment-specific config leakage)
Also applies to: 58-67
69-78: WASI exclusion plugin correctly prevents WASI build bundling.The plugin filters out
tailwindcss-oxide.wasi.cjsby returning empty contents, which aligns with the PR objective to exclude WASI builds from the standalone binary and works in conjunction with theNAPI_RS_FORCE_WASIdefine.
|
Apparently setting |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/@tailwindcss-standalone/scripts/build.ts (2)
13-13: Optional: Simplify redundant USERPROFILE check.The condition
process.env.USERPROFILE !== ''is redundant because the preceding checkprocess.env.USERPROFILEalready ensures the variable is truthy (non-empty and defined).Apply this diff to simplify:
-if (process.env.NESTED_BUILD !== '1' && process.env.USERPROFILE && process.env.USERPROFILE !== '') { +if (process.env.NESTED_BUILD !== '1' && process.env.USERPROFILE) {
40-101: Consider adding error handling for individual build failures.The build loop doesn't wrap individual builds in try-catch blocks, meaning a single platform build failure will terminate the entire process without producing artifacts for subsequent platforms. While fail-fast behavior might be intentional, adding error handling could improve resilience by allowing successful builds to complete and reporting all failures at the end.
If you want to allow partial success, consider wrapping each build iteration:
let errors: Array<{ target: string; error: Error }> = [] for (let { target, name } of builds) { try { // ... existing build logic ... } catch (error) { errors.push({ target: name, error: error as Error }) console.error(`Build failed for ${name}:`, error) continue } } if (errors.length > 0) { console.error(`\n${errors.length} build(s) failed`) process.exit(1) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@tailwindcss-standalone/scripts/build.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: macOS / webpack
- GitHub Check: macOS / cli
- GitHub Check: macOS / oxide
- GitHub Check: macOS / upgrade
- GitHub Check: Linux / webpack
- GitHub Check: Linux / upgrade
- GitHub Check: Linux / oxide
- GitHub Check: Linux / postcss
- GitHub Check: Linux / vite
- GitHub Check: Linux / cli
- GitHub Check: Windows / upgrade
- GitHub Check: Windows / postcss
- GitHub Check: Windows / webpack
- GitHub Check: Windows / oxide
- GitHub Check: Windows / vite
- GitHub Check: Windows / cli
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: Linux
🔇 Additional comments (3)
packages/@tailwindcss-standalone/scripts/build.ts (3)
24-41: LGTM! Well-structured multi-target build configuration.The target array clearly defines all platform variants with appropriate naming. The baseline builds for x64 ensure compatibility with older hardware, and the type mismatch is appropriately acknowledged.
107-107: LGTM! Checksum format correctly fixed.The checksum file format now correctly uses
./${name}without adding an extratailwindcss-prefix, as noted in the past review comment.
53-66: No action needed. ThePLATFORM_LIBCenvironment variable is only referenced in Linux-specific code paths (guarded byprocess.platform === 'linux'checks in the lightningcss and @parcel/watcher patches). Setting it for all platforms with a default value of'glibc'is safe and does not cause issues on macOS or Windows.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/@tailwindcss-standalone/scripts/build.ts (1)
79-88: Consider specifying the loader type explicitly.The plugin returns empty contents without specifying a loader. While Bun may handle this correctly, explicitly adding
loader: 'js'would make the intent clearer.setup(build) { build.onLoad({ filter: /tailwindcss-oxide\.wasi\.cjs$/ }, async (args) => { - return { contents: '' } + return { contents: '', loader: 'js' } }) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@tailwindcss-standalone/scripts/build.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: macOS / oxide
- GitHub Check: macOS / webpack
- GitHub Check: macOS / cli
- GitHub Check: macOS / upgrade
- GitHub Check: Linux / postcss
- GitHub Check: Linux / vite
- GitHub Check: Linux / cli
- GitHub Check: Windows / webpack
- GitHub Check: Linux / upgrade
- GitHub Check: Windows / cli
- GitHub Check: Windows / oxide
- GitHub Check: Windows / postcss
- GitHub Check: Windows / upgrade
- GitHub Check: Windows / vite
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: macOS
🔇 Additional comments (2)
packages/@tailwindcss-standalone/scripts/build.ts (2)
8-20: Windows workaround is appropriately documented and implemented.The nested process spawn to clear USERPROFILE addresses a specific Bun binary download issue on Windows CI. The comment clearly explains why runtime modification doesn't work.
107-114: Checksum format and output are correct.The checksum format correctly uses the name as-is without adding a duplicate prefix, and the summary output provides useful build information.
|
Some edit: This was just a local environment problem. |
Fixes #19389
We inlined env vars in the Standalone CLI because we use some custom patches + env vars to ensure that only the appropriate
glibc/muslbinaries are included for Lightning CSS and Parcel Watcher for Linux builds.The build happens to run on a macOS Github CI machine though so
NODE_PATHwas getting inlined as the string:I don't think there's a reason for
NODE_PATHto work on the Standalone CLI (and it didn't work because of the above bug anyway) so I've done a few things here:Bun.build(…)which now supports compiling binaries. This speeds up the build process a bit.define.NODE_PATHsupport in@tailwindcss/nodewhen building with the Standalone CLI.__tw_readFilehack is now gone. Async FS APIs were not originally able to read embedded files but that changed in Bun v1.2.3 making the hack unnecessary.I want to find a way to get rid of
__tw_resolveand__tw_loadbut don't want to change too much in this PR so I haven't looked into it yet.Need to test all platforms so: [ci-all]