fix: error messages in esbuild node module plugin #264
Conversation
🦋 Changeset detectedLatest commit: aad25e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Node.js module detection plugin to provide more accurate error reporting with source location information when workflow functions attempt to use Node.js built-in modules or npm packages that depend on them.
- Adds comprehensive location tracking to pinpoint exact usage of incompatible modules in workflow code
- Implements import chain analysis to detect Node.js modules used transitively through npm packages
- Adds
node-fetchdependency and test violations to workbench to validate the new detection capabilities
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| workbench/sveltekit/workflows/user-signup.ts | Adds test violations using readFileSync and node-fetch to validate plugin detection |
| workbench/sveltekit/package.json | Adds node-fetch dependency for testing plugin's npm package violation detection |
| pnpm-lock.yaml | Updates lockfile with node-fetch and its dependencies |
| packages/builders/src/node-module-esbuild-plugin.ts | Major refactor adding location tracking, import chain analysis, and helper functions for accurate violation reporting |
| packages/builders/src/node-module-esbuild-plugin.test.ts | Comprehensive test updates with unit tests for helper functions and integration tests for violation detection with location information |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| it('should return undefined for files without the package import', () => { | ||
| const cwd = process.cwd(); | ||
| const testFile = 'src/node-module-esbuild-plugin.test.ts'; |
There was a problem hiding this comment.
The test file path 'src/node-module-esbuild-plugin.test.ts' is incorrect. Since process.cwd() returns the monorepo root, the correct relative path should be 'packages/builders/src/node-module-esbuild-plugin.test.ts'. This test will fail when run from the monorepo root.
| for (let i = startIndex; i < lines.length; i += 1) { | ||
| const line = lines[i]; | ||
|
|
||
| // Skip comments (both // and /* */ style) | ||
| const withoutComments = line | ||
| .replace(/\/\*[\s\S]*?\*\//g, '') | ||
| .replace(/\/\/.*$/, ''); | ||
|
|
There was a problem hiding this comment.
The regex /\/\*[\s\S]*?\*\//g may not correctly handle multi-line comments that span across different lines in the array, since each line is processed individually. A multi-line comment starting on one line and ending on another won't be properly stripped, potentially causing false positives or negatives in identifier detection.
| for (let i = startIndex; i < lines.length; i += 1) { | |
| const line = lines[i]; | |
| // Skip comments (both // and /* */ style) | |
| const withoutComments = line | |
| .replace(/\/\*[\s\S]*?\*\//g, '') | |
| .replace(/\/\/.*$/, ''); | |
| // Join all lines to a single string and remove multi-line comments | |
| const codeWithoutBlockComments = lines.join('\n').replace(/\/\*[\s\S]*?\*\//g, ''); | |
| // Split back into lines | |
| const processedLines = codeWithoutBlockComments.split('\n'); | |
| for (let i = startIndex; i < processedLines.length; i += 1) { | |
| const line = processedLines[i]; | |
| // Remove single-line comments | |
| const withoutComments = line.replace(/\/\/.*$/, ''); |
| .replace(/'[^']*'/g, (segment) => ' '.repeat(segment.length)) | ||
| .replace(/"[^"]*"/g, (segment) => ' '.repeat(segment.length)) | ||
| .replace(/`[^`]*`/g, (segment) => ' '.repeat(segment.length)); |
There was a problem hiding this comment.
The string literal removal using /[^']*'/, /[^"]*"/, and /[^]*/ won't handle escaped quotes properly. For example, strings like 'it\\'s' or "say \\"hello\\"" won't be matched correctly, potentially causing the identifier search to match inside string content.
|
|
||
| it('should return undefined when import is unused even if it can be parsed', () => { | ||
| const cwd = process.cwd(); | ||
| const testFile = 'src/node-module-esbuild-plugin.test.ts'; |
There was a problem hiding this comment.
The test file path 'src/node-module-esbuild-plugin.test.ts' is incorrect. Since process.cwd() returns the monorepo root, the correct relative path should be 'packages/builders/src/node-module-esbuild-plugin.test.ts'. This test will fail when run from the monorepo root.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| relativeWorkflowFile, | ||
| packageName | ||
| ); | ||
| if (location) { |
There was a problem hiding this comment.
Violations without location information are silently ignored. If getViolationLocation returns undefined, the violation is never reported, allowing builds with Node.js module violations to succeed.
View Details
📝 Patch Details
diff --git a/packages/builders/src/node-module-esbuild-plugin.ts b/packages/builders/src/node-module-esbuild-plugin.ts
index 9f220b3..521a768 100644
--- a/packages/builders/src/node-module-esbuild-plugin.ts
+++ b/packages/builders/src/node-module-esbuild-plugin.ts
@@ -18,7 +18,7 @@ type PackageViolation = {
packageName: string;
importer: string;
path: string;
- location: Partial<esbuild.Location>;
+ location?: Partial<esbuild.Location>;
};
/*
@@ -260,14 +260,12 @@ export function createNodeModuleErrorPlugin(): esbuild.Plugin {
relativeWorkflowFile,
packageName
);
- if (location) {
- packageViolations.push({
- path: args.path,
- importer: relativeWorkflowFile,
- packageName,
- location,
- });
- }
+ packageViolations.push({
+ path: args.path,
+ importer: relativeWorkflowFile,
+ packageName,
+ location,
+ });
}
return {
Analysis
Node.js module violations with missing location information are silently ignored
What fails: The createNodeModuleErrorPlugin in packages/builders/src/node-module-esbuild-plugin.ts silently ignores Node.js module violations when getViolationLocation() returns undefined (e.g., for multiline imports). Violations are only added to packageViolations if a location is found (line 263), causing the build to succeed without reporting errors when location information cannot be determined.
How to reproduce:
// Create a file with a multiline import of a Node.js module
import {
readFile,
writeFile
} from "fs";
export function workflow() {
return readFile("test.txt");
}Run esbuild with createNodeModuleErrorPlugin(). The build succeeds even though it contains a prohibited Node.js module import. Single-line imports are correctly detected and reported.
Result: Build succeeds without error. Multiline imports are not reported.
Expected: Build should fail with an error reporting the Node.js module violation, even if precise location information cannot be determined.
Root cause: The getViolationLocation() function uses a line-by-line regex that fails to match multiline import statements. When location is undefined, the violation is not added to packageViolations due to the conditional check. The onEnd handler only reports errors if packageViolations.length > 0, so violations without locations are never reported.
Fix implemented: Removed the if (location) conditional check and made the location field optional in the PackageViolation type. Violations are now always added to packageViolations array, regardless of whether location information could be determined. The error mapping already handles undefined locations correctly by omitting location details from the error object.
adriandlam
force-pushed
the
fix/node-module-plugin-traces
branch
from
0dd9662 to
b62baf0
Compare
adriandlam
force-pushed
the
fix/node-module-plugin-traces
branch
from
b62baf0 to
98c23dc
Compare
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
esbuild node module plugin now:
node_modules) inside workflows