fix(build): resolve app imports of astryx to source in withAstryx - #5932
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The scoped rule withAstryx installs carries the `source` condition on
`Rule.resolve`, but `Rule.test` matches the module being processed and
`Rule.resolve` governs the requests that module makes. An app's own
`@astryxdesign/*` imports are issued from its sources, outside
node_modules, so they never match the rule and resolve through `default`
to dist.
The dist runtime emits `x`-prefixed atomic class names while the PostCSS
pass compiles the library from source and emits `astryx`-prefixed rules.
The two sets are disjoint, so the build exits 0, the route prerenders, a
full stylesheet is served, and the page renders unstyled with nothing
logged. apps/example-nextjs-source uses `withAstryx({})` as written, so
following it ships an unstyled site.
Widening the global conditionNames fixes resolution but breaks React's
JSX resolution and mis-resolves third-party packages that also publish a
`source` condition, which is what the scoped rule exists to avoid.
Webpack rules cannot key on the request string, so map the packages'
`source` export targets to aliases instead: resolution follows the astryx
packages themselves and the global conditions stay as Next resolved them.
Subpaths get an entry each, since `@astryxdesign/core/AlertDialog` is as
much a documented entry point as the root.
Fixes facebook#5920
d924b3b to
c018a31
Compare
cixzhang
left a comment
There was a problem hiding this comment.
Thanks for fixing the silent unstyled build. One compatibility issue still blocks this: an ordinary caller alias such as {'@astryxdesign/core': customPath} loses to the generated exact aliases, so composed Next configs silently load Astryx source instead of the caller's implementation. Please preserve both exact and prefix caller aliases; a resolver-level regression test would cover the real behavior.
Also please run Prettier on the two changed source files.
[Reviewed by Robohands]
The generated entries were spread before the caller's, so a composed config that aliases `@astryxdesign/core` to its own implementation lost to the exact `@astryxdesign/core$` entry this helper adds: webpack's alias list is ordered and first match wins, and a caller prefix alias is a different key, so it sat behind ours instead of replacing it. The app then silently loaded astryx source in place of the caller's module. Drop any generated alias the caller has already spoken for. A prefix alias claims the package and every subpath under it; an exact `$` alias claims only the bare specifier, so the generated subpaths survive it. An array-shaped `resolve.alias` is now carried through as an array — spreading one into an object turned it into index keys and broke resolution outright. Tests resolve through enhanced-resolve, the resolver webpack runs, since precedence is an ordering property of the resolver rather than of the config object.
|
good catch, and it was worse than the exact/prefix mismatch. now dropping any generated alias the caller has already spoken for: a prefix alias claims the moved the tests onto one thing to flag — prettier run on both files, |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks for fixing the exact, ordinary prefix, and array alias cases; those now pass through the real resolver. Two compatibility gaps remain:
- Apps using an object wildcard alias like
{'@astryxdesign/*': custom}still load Astryx source instead of their configured implementation because generated exact aliases stay first (packages/build/src/next.js:53). - Contributors on macOS get 6 of 13 resolver tests failing because the fixture compares
/var/...with the canonical/private/var/...result (packages/build/src/next.test.mjs:42).
Could you preserve wildcard caller aliases too and canonicalize the fixture root? If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…e root
A caller entry such as `{'@astryxdesign/*': custom}` was not recognized
as claiming the scope, so the generated exact entries stayed ahead of it
and the app loaded astryx source instead of the configured
implementation. Wildcard keys now match the requests they cover, the
same way the exact and prefix forms already did.
The fixture compared paths against the `mkdtemp` root while the resolver
reports the canonical one. On macOS the temp dir sits under `/var`, a
symlink to `/private/var`, so the two never matched and the resolver
assertions failed for contributors on that platform. Taking the real
path of the fixture root fixes it; a symlinked TMPDIR reproduces the
failure on Linux and passes with the change.
|
both confirmed and fixed in a280736. wildcard first — i checked the resolver actually honors worth noting the two target shapes behave differently and i tested both — a plain target for the macOS one i couldn't hit it directly on linux, but pointing TMPDIR at a symlink fixture root is prettier clean on both files. thanks for the discord offer — nothing blocking on my side, but |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks, the wildcard and macOS fixes themselves are good. One composition path still has the original compatibility bug: withAstryx() merges generated aliases before it calls the caller’s webpack hook (packages/build/src/next.js:209-225). When that hook adds an ordinary prefix or wildcard alias, enhanced-resolve still picks the earlier generated exact entries and loads Astryx source; only the identical exact $ key wins. Could we preserve exact, prefix, and wildcard aliases configured through the hook too, with resolver-level coverage for that real Next composition path? If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
The merge ran before `existingWebpack`, so an alias the caller's own hook added landed behind the generated entries in the alias list. The resolver takes the first match, so a prefix or wildcard alias contributed from the hook lost and the app loaded astryx source instead of the configured implementation; only a byte-identical `$` key replaced ours outright. Run the hook first and merge against the config it returns, so aliases from either side get the same precedence. A hook that returns nothing now falls back to the config it was handed rather than throwing. Covered through enhanced-resolve on the composition path itself: prefix, wildcard, and exact aliases added from a caller hook, plus a generated entry the hook leaves alone.
|
you're right, and it was the ordering rather than the matching — fixed in 6cf541e by running the hook first and merging against the config it returns, so and after, all three land on resolver-level coverage for that path specifically — prefix, wildcard and exact from a caller one extra thing that fell out of it: a hook returning nothing used to throw, since the return 21 assertions, passing under both a normal and a symlinked TMPDIR. prettier clean. |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks, this now preserves caller aliases through both direct config and the webpack hook. Resolver coverage, macOS paths, formatting, and exact-head CI all pass.
[Reviewed by Robohands]
withAstryx()installs a rule that carries thesourcecondition onRule.resolve, scopedby
testtonode_modules/@astryxdesign/. ButRule.testmatches the module beingprocessed and
Rule.resolvegoverns the requests that module makes, so the rule onlycovers astryx-to-astryx imports. An app's own
@astryxdesign/*imports are issued from itssources, outside
node_modules, so they never match and resolve throughdefaultto dist.Dist's runtime emits
x-prefixed atomic class names while the PostCSS pass compiles thelibrary from source and emits
astryx-prefixed rules. The two sets share no class names, sothe build exits 0, the route prerenders, a full-size stylesheet is served, and the page
renders unstyled with nothing logged.
apps/example-nextjs-sourceuseswithAstryx({})aswritten, so a team following it ships an unstyled site.
Widening the global
conditionNamesdoes fix resolution, but it breaks React's JSXresolution and mis-resolves third-party packages that also publish a
sourcecondition(
lexical) — which is exactly what the scoped rule exists to prevent. Webpack rules cannotkey on the request string, so there is no rule shape that expresses "requests for
@astryxdesign/*from app code". Instead this reads thesourcetargets out of eachinstalled astryx package's export map and sets them as
resolve.aliasentries: resolutionfollows the astryx packages themselves and the global conditions stay as Next resolved them.
Subpaths get an entry each, since
@astryxdesign/core/AlertDialogis as much a documentedentry point as the root — aliasing only the root would leave 119 subpaths on dist and produce
a mixed bundle. A user-supplied alias still wins, and packages that are absent or ship no
sourcecondition keep normal resolution.Note that
require.resolve('@astryxdesign/core/package.json')throwsERR_PACKAGE_PATH_NOT_EXPORTED— core'sexportshas no./package.jsonkey — so themanifest lookup walks
node_modulesthe way Node resolves a bare specifier.On verification, being precise about what I did and did not run. I added
packages/build/src/next.test.mjs, 9 assertions over the configwithAstryx()emits againsta fixture package laid out the way npm installs one. It fails on 5 of them before this change
and passes all 9 after. I could not run it through the repo's own vitest —
pnpm installpulls the full monorepo toolchain and I deliberately kept build tooling off this machine — so
I executed the same assertions directly against the module under Node; CI will be the first
run through vitest proper.
check:changesets,check:cli-structure,check:portable-scripts,check:executable-bitsandcheck:use-clientpass locally.I have not reproduced the unstyled-page symptom end to end. There is no
next buildhere, sothe class-prefix mismatch itself is verified from the resolution semantics and the reporter's
measurements rather than observed. The reporter has a harness that automates this across every
documented setup and both bundlers; pointing it at this branch would be a stronger signal than
anything I can produce locally.
Turbopack is untouched —
withAstryx()writesnextConfig.webpack, which Turbopack ignores,and that is filed separately as #5921.
Fixes #5920