Skip to content

feat(server-utils): Warn when bundler config has instrumented module in external#22379

Open
timfish wants to merge 1 commit into
developfrom
timfish/feat/orchestrion-bunlers
Open

feat(server-utils): Warn when bundler config has instrumented module in external#22379
timfish wants to merge 1 commit into
developfrom
timfish/feat/orchestrion-bunlers

Conversation

@timfish

@timfish timfish commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

If users have a library we instrument in their bundler external config we should warn them about the fact that this stops it from getting instrumented!

@timfish
timfish requested a review from a team as a code owner July 19, 2026 16:25
@timfish
timfish requested review from isaacs, msonnb, mydea and stephanie-anderson and removed request for a team July 19, 2026 16:25
Comment thread packages/server-utils/src/orchestrion/bundler/vite.ts
@andreiborza
andreiborza removed the request for review from stephanie-anderson July 20, 2026 08:58

@isaacs isaacs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
*/
/**

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't realize it did that. Very convenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants