-
Notifications
You must be signed in to change notification settings - Fork 259
fix(builders): resolve path aliases when externalizing non-step imports #1613
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
Changes from all commits
7c8e89b
c92904d
a869505
60ed9e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@workflow/builders": patch | ||
| --- | ||
|
|
||
| Resolve path aliases when externalizing non-step imports |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,8 @@ export function createSwcPlugin(options: SwcPluginOptions): Plugin { | |
| ); | ||
|
|
||
| build.onResolve({ filter: /.*/ }, async (args) => { | ||
| if (args.pluginData?.skipSwcPlugin) return null; | ||
|
|
||
| if ( | ||
| !options.entriesToBundle && | ||
| normalizedSideEffectEntries.size === 0 | ||
|
|
@@ -122,21 +124,44 @@ export function createSwcPlugin(options: SwcPluginOptions): Plugin { | |
| } | ||
|
|
||
| try { | ||
| let resolvedPath: string | false | undefined = args.path; | ||
| const specifier = args.path; | ||
| const specifierIsPath = | ||
| specifier.startsWith('.') || specifier.startsWith('/'); | ||
|
|
||
| let resolvedPath: string | false | undefined; | ||
| // Determines whether the external path should be relativized | ||
| // (project-local file) or kept as a bare specifier (npm package). | ||
| let shouldMakeRelative = specifierIsPath; | ||
|
|
||
| // handle local imports e.g. ./hello or ../another | ||
| if (args.path.startsWith('.')) { | ||
| resolvedPath = await enhancedResolve(args.resolveDir, args.path); | ||
| if (specifierIsPath) { | ||
| resolvedPath = await enhancedResolve(args.resolveDir, specifier); | ||
| } else { | ||
| // Resolve from project root so nested deps aren't externalized | ||
| resolvedPath = await enhancedResolve( | ||
| // `args.resolveDir` is not used here to ensure we only | ||
| // externalize packages that can be resolved in the | ||
| // project's working directory e.g. a nested dep can't | ||
| // be externalized as we won't be able to resolve it once | ||
| // it's parent has been bundled | ||
| build.initialOptions.absWorkingDir || process.cwd(), | ||
| args.path | ||
| ); | ||
| specifier | ||
| ).catch(() => undefined); // swallow so esbuild fallback below can try | ||
|
|
||
| // Fall back to esbuild for aliases/tsconfig paths, | ||
| // but only accept project-local results | ||
| if (!resolvedPath) { | ||
| const esbuildResult = await build.resolve(specifier, { | ||
| resolveDir: args.resolveDir, | ||
| kind: args.kind, | ||
| pluginData: { skipSwcPlugin: true }, | ||
| }); | ||
| const didResolve = | ||
| !!esbuildResult.path && !esbuildResult.errors.length; | ||
| const isProjectLocalFile = | ||
| didResolve && | ||
| !esbuildResult.path | ||
| .replace(/\\/g, '/') | ||
| .includes('/node_modules/'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI Review: NitThe |
||
| if (isProjectLocalFile) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AI Review: NoteWhen the esbuild fallback resolves an aliased path that also appears in |
||
| resolvedPath = esbuildResult.path; | ||
| shouldMakeRelative = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!resolvedPath) return null; | ||
|
|
@@ -182,11 +207,8 @@ export function createSwcPlugin(options: SwcPluginOptions): Plugin { | |
| : null; | ||
| } | ||
|
|
||
| const isFilePath = | ||
| args.path.startsWith('.') || args.path.startsWith('/'); | ||
|
|
||
| let externalPath: string; | ||
| if (isFilePath) { | ||
| if (shouldMakeRelative) { | ||
| externalPath = relative( | ||
| options.outdir || process.cwd(), | ||
| resolvedPath | ||
|
|
@@ -201,7 +223,7 @@ export function createSwcPlugin(options: SwcPluginOptions): Plugin { | |
| .replace(/\.cts$/, '.cjs'); | ||
| } | ||
| } else { | ||
| externalPath = args.path; | ||
| externalPath = specifier; | ||
| } | ||
|
|
||
| return { | ||
|
|
||
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.
AI Review: Note
This test uses esbuild
aliasto set up the path alias, but the more common real-world scenario is tsconfigpaths(which is how most Next.js / Vite projects configure@/*aliases). Consider adding a test that uses atsconfig.jsonwithcompilerOptions.pathsand passes it via esbuild'stsconfigoption — this would exercise the samebuild.resolve()fallback path but through the tsconfig resolution pipeline that most users actually hit.I verified locally that this does work (wrote a test using
tsconfig: join(testRoot, 'tsconfig.json')withpaths: { "@/*": ["./src/*"] }instead ofalias), but having it in the test suite would increase confidence.