Repository navigation
test: stop relying on pnpm's NODE_PATH in build and core tests - #6372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Addresses the review on facebook#6372. - ./postcss required postcss only to parse its generated CSS. It now passes replaceWith() a string, which the host's own postcss parses, so both the require and the optional peer the earlier commit declared go away. - ./vite required lightningcss and browserslist from its own location, where neither is declared, and its try/catch turned the miss into a silently skipped pass. It now resolves both from @stylexjs/unplugin, which depends on them and whose output that pass reproduces. The test guarding the pass looked for any -webkit- prefix, which an authored one always satisfied. It now checks a prefix only the pass adds, and fails on the old code without NODE_PATH. - apps/sandbox declares autoprefixer. build's postcss() config adds it, and Next resolves it from the app.
Addresses the review on facebook#6372. - ./postcss required postcss only to parse its generated CSS. It now passes replaceWith() a string, which the host's own postcss parses, so both the require and the optional peer the earlier commit declared go away. - ./vite required lightningcss and browserslist from its own location, where neither is declared, and its try/catch turned the miss into a silently skipped pass. It now resolves both from @stylexjs/unplugin, which depends on them and whose output that pass reproduces. The test guarding the pass looked for any -webkit- prefix, which an authored one always satisfied. It now checks a prefix only the pass adds, and fails on the old code without NODE_PATH. - apps/sandbox declares autoprefixer. build's postcss() config adds it, and Next resolves it from the app.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Visual Regression25 of 420 shot(s) changed. View the report A change here is a question, not a failure: check whether the after is the
and 5 more. Chat — Full AI Chat — probe light
Chat — Empty State — probe light
Chat — Full AI Chat — probe dark
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
AI review status for this pull request.
|
There was a problem hiding this comment.
This change makes each package resolve only what it declares, so strict installs stop breaking: the PostCSS entry hands the host parser a CSS string, the Vite entry resolves lightningcss/browserslist from @stylexjs/unplugin, the missing enhanced-resolve/autoprefixer/next declarations are added with no new lockfile versions, and the dead lexical$ alias is removed. That all looks right.
One thing to fix before approval: the CLI MultiSelectorColumnVisibilitySelector template drops its formatValue prop (plus a doc tweak), which changes the generated block's trigger text from "N columns shown" to the component default "N selected". It's unrelated to this fix and isn't mentioned in the PR description, so please revert those two template files here and move that copy change to its own change — or explain in the description why it belongs here.
Also non-blocking: core pins next exactly at a preview release while the sandbox tracks ^15; fine for tests, but consider a caret or catalog entry so it doesn't rot.
[Automated review]
|
Thanks! This PR doesn't touch I think the diff used the PR's recorded |
|
On the non-blocking
|
There was a problem hiding this comment.
This fix makes each package resolve only what it declares, so strict installs stop breaking: the PostCSS entry hands the host parser a CSS string instead of requiring its own postcss, the Vite entry resolves lightningcss/browserslist from @stylexjs/unplugin, enhanced-resolve and autoprefixer are now declared where they are resolved, core tests resolve their Babel presets from core, and the Selector source-build test guards the arrow-lowering case without needing next/babel. The dead sandbox lexical$ alias is also removed, and the lockfile adds no new package versions.
The two points from the earlier review are addressed in this revision: the CLI template files are not changed by this PR (that difference came from the recorded base), and core no longer depends on next. With the stronger Vite test checking a prefix only the lightningcss pass adds, this looks ready to approve.
[Automated review]
|
Thanks @Han5991 for tracking down the strict-resolution / We validated the published-package path separately and landed #6605, which now owns the consumer-facing Could you rebase this PR onto current
The Build PostCSS/Vite ownership changes and Build changeset are superseded by #6605. The sandbox Once rebased, we’ll let fresh CI and CodeBunny run on the new head and re-review the remaining change. Thanks again! |
…DE_PATH After facebook#5327 made node_modules strict, packages/build/src/next.test.mjs kept resolving enhanced-resolve (facebook#5932) without a declaration: it arrives only transitively through next, and pnpm's vitest shim puts node_modules/.pnpm/node_modules on NODE_PATH, which require folds in. Run without it, the test fails with MODULE_NOT_FOUND. Declare it as a build dev dependency at the version the lockfile already carries, so the lockfile gains an importer entry and no new package versions. The other two requires this commit first declared are covered elsewhere: facebook#6605 has ./postcss let the host's postcss parse its CSS, and facebook#6627 removed the sandbox's next.config.mjs.
ButtonGroup.test.tsx and Selector.source-build.test.mjs hand Babel bare preset names. Babel looks those up from the cwd -- the repo root, which declares none of them -- so they resolved only through the node_modules/.pnpm/node_modules entry pnpm's vitest shim puts on NODE_PATH. Without it, the first fails on @babel/preset-typescript and the second on next. Resolve each preset from the test's own package instead. core already declares both Babel presets; it now declares next too, at the 16.3.0-preview.5 the test was already running against, so the lockfile reuses the existing snapshot.
postProcessCss catches any failure and returns the CSS unprocessed, so the test guarding that pass has to see something only the pass produces. It looked for any -webkit- prefix, which the -webkit-box-orient authored in source always satisfies: with the pass forced to skip, it stays green. Check for -webkit-user-select, which only the pass adds and which fails when it is skipped. This commit first also had ./postcss and ./vite resolve only what build can reach itself. facebook#6605 covers that, along with a packed-consumer test of the same pass; this keeps the in-repo build test honest too.
The Selector source-build test guards one thing: a consumer preset lowering module-scope arrows to function expressions, which StyleX cannot evaluate (facebook#5464). A plugin that does only that reproduces it, so core no longer needs next, and there is no exact preview pin to drift from docsite's. With linePad restored, the test still fails with facebook#5464's error (Unsupported expression: FunctionExpression). Across all 226 core files that call stylex.create, next/babel and this pipeline fail the same set: none.
b182bf4 to
0dc1aec
Compare
|
Thanks @cixzhang, and for landing #6605. I rebased onto current
I updated the description to match. |
There was a problem hiding this comment.
This change stops the build and core tests from relying on pnpm's NODE_PATH shim, so each package resolves only what it declares. The build package now declares the enhanced-resolve its Next.js test requires (the lockfile only gains an importer entry, with no new package versions), the ButtonGroup and Selector tests resolve their Babel presets from the core package that declares them, and the Selector source-build test replaces next/babel with those presets plus a small plugin that lowers arrows — which is the effect the regression guard actually needs. The Vite build test is also stronger: it now checks for a vendor prefix only the lightningcss pass adds, instead of one that was already authored in source.
No public API or product behavior changes, and the earlier template and next-pin points do not apply to this revision — no template files are touched and core no longer depends on next. This looks ready to approve.
[Automated review]
Maintainer impact
pnpm's command shims put
node_modules/.pnpm/node_modulesonNODE_PATH. Undervitest, that lets three tests resolve packages their own package does not declare, and each fails when run without it:packages/build/src/next.test.mjs→enhanced-resolve(fix(build): resolve app imports of astryx to source in withAstryx #5932), which reached build only throughnextButtonGroup.test.tsx→@babel/preset-typescript, passed to Babel by bare name and looked up from the cwd (the repo root)Selector.source-build.test.mjs→next(asnext/babel), which core does not declareThe shipped
@astryxdesign/buildside of this is covered by #6605. This PR is limited to the tests.Intended invariant
A package resolves only what it declares (#5327).
Change
enhanced-resolveas a build dev dependency. The lockfile gains an importer entry and no new package versions.ButtonGroup.test.tsxresolves its Babel presets from core, which declares them.Selector.source-build.test.mjsno longer runsnext/babel. It uses core's TypeScript and React presets plus a small plugin that lowers arrows to function expressions, which is all Selector's linePad helper breaks any consumer that runs next/babel over the package source #5464 needs.vite.build.test.tschecks for-webkit-user-select, a prefix only the lightningcss pass adds. The old-webkit-check also matched the-webkit-box-orientauthored in source.Evidence
NODE_PATH, the three tests fail withCannot find module 'enhanced-resolve',Cannot find module '@babel/preset-typescript', andCannot find package 'next'. With the lightningcss pass forced to skip, the old vite assertion stays green.NODE_PATH. With the pass forced to skip, the new vite assertion fails. With Selector's linePad helper breaks any consumer that runs next/babel over the package source #5464'slinePadhelper restored, the Selector test still fails withUnsupported expression: FunctionExpression. Across all 226 core files that callstylex.create,next/babeland the new pipeline fail the same set: none.Scope
Testing
pnpm install --frozen-lockfilenode node_modules/vitest/vitest.mjsandNODE_PATHunset (the pnpm shim always sets it):packages/build/src(64 passed),ButtonGroup.test.tsx,Selector.source-build.test.mjsmainversion run the same way, to confirm the failures abovepnpm -F @astryxdesign/core typecheck, eslint and prettier on the changed files, andcheck:repo(commit hook)