chore: esbuild minify & comment stripping#39
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds esbuild to the build pipeline for minification and comment stripping, claiming to reduce bundle sizes dramatically (e.g., result.js from 27k to 3.5k). The PR also includes improvements to the optional lint rule to better distinguish between String.prototype.match and custom .match() methods.
Key Changes:
- Added esbuild v0.25.10 as a dev dependency with minification and source map generation in the build script
- Enhanced lint rule to detect string-like objects using heuristics to avoid false positives with custom .match() methods
- Bumped package version from 1.6.0 to 1.7.0
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| package.json | Added esbuild dependency and integrated it into the build script with minification, comment stripping, and sourcemap generation |
| package-lock.json | Added esbuild and its platform-specific dependencies; updated version to 1.7.0; removed "peer": true flags from several dependencies |
| src/optional/lint.ts | Added heuristic logic to distinguish String.prototype.match from custom .match() methods by checking if the caller object appears to be a string |
| src/optional/lint.test.ts | Added test case to verify custom .match() methods don't trigger the optional usage rule |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@typescript-eslint/parser": "^8.39.0", | ||
| "@typescript-eslint/rule-tester": "^8.39.0", | ||
| "@typescript-eslint/utils": "^8.39.0", | ||
| "esbuild": "^0.25.10", |
There was a problem hiding this comment.
The package.json specifies esbuild version ^0.25.10, but the package-lock.json has locked it to 0.25.12. While this is acceptable due to the caret range, consider updating package.json to ^0.25.12 to match the actual installed version for clarity and to ensure all developers use the same version during development.
| "esbuild": "^0.25.10", | |
| "esbuild": "^0.25.12", |
| }, | ||
| "scripts": { | ||
| "build": "tsc --project tsconfig.build.json", | ||
| "build": "tsc --project tsconfig.build.json && esbuild \"dist/**/*.js\" --outdir=dist --outbase=dist --format=esm --minify --legal-comments=none --sourcemap --allow-overwrite", |
There was a problem hiding this comment.
The esbuild command uses a glob pattern "dist/**/*.js" which may have unintended behavior across different shells. Consider using a more explicit approach or ensuring the glob is properly expanded. Additionally, the --allow-overwrite flag means esbuild will overwrite the TypeScript-compiled output. If the build fails at the esbuild step, you might be left with partially processed files. Consider using a separate output directory for the minified files or adding proper error handling.
| /^(text|str|string|content|message|input|output|name|value|data|source|target)s?$/i.test( | ||
| object.name | ||
| )); |
There was a problem hiding this comment.
The regex pattern includes 's?' at the end to optionally match plural forms, but this could lead to false negatives. For example, 'texts' would match, but 'messages' would not because the regex expects the optional 's' after 'message', not 'messages' (which already ends in 's'). The pattern should be adjusted to properly handle both singular and plural forms of words that already end in 's', or the pattern should be reconsidered to avoid this inconsistency.
| { | ||
| name: "Custom .match() method (not String.prototype.match) should not trigger", | ||
| code: `const result = myMatcher.match("pattern"); | ||
| const result2 = customObject.match(/regex/); | ||
| const result3 = patternMatcher.match(\`template\`);`, | ||
| }, |
There was a problem hiding this comment.
The test case for custom .match() methods doesn't cover the new heuristic logic that checks for string-like identifier names. Consider adding test cases that verify the behavior when identifiers match the string-like pattern (e.g., 'text.match()', 'message.match()') to ensure these are correctly identified as String.prototype.match calls and DO trigger the rule if they return nullable values.
| // Check if the object looks like it's a string to avoid false positives | ||
| // with custom .match() methods (e.g., Result.match, Matcher.match) | ||
| const object = node.callee.object; | ||
| const isLikelyString = | ||
| // String literal: "text".match(...) | ||
| (object.type === AST_NODE_TYPES.Literal && | ||
| typeof object.value === "string") || | ||
| // Template literal: `text ${var}`.match(...) | ||
| object.type === AST_NODE_TYPES.TemplateLiteral || | ||
| // Identifier with string-like name that suggests it's a string | ||
| (object.type === AST_NODE_TYPES.Identifier && | ||
| /^(text|str|string|content|message|input|output|name|value|data|source|target)s?$/i.test( | ||
| object.name | ||
| )); | ||
|
|
||
| if (!isLikelyString) return false; |
There was a problem hiding this comment.
This PR mixes two unrelated changes: (1) adding esbuild for minification as stated in the title, and (2) improving the lint rule's detection of String.prototype.match vs custom .match() methods. These should ideally be separate PRs to maintain clear change history and make reviews more focused. If these changes are intentionally bundled together, the PR title and description should be updated to reflect both changes.
|
🎉 This PR is included in version 1.7.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
reduce bundle sizes dramatically. IE result.js 27k => 3.5k