fix(bundler-plugins): integration with monorepo#21342
Conversation
Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
…ludeTracing` (#644) Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
This reverts commit 8eb6c5f.
Revert the default behaviour of the webpack plugin to no longer exit the process. Instead, users can set an experimental flag to force exiting the process.
…terUpload` (#677) Widen the accepted type for `filesToDeleteAfterSourcemaps` to allow us (as well as users) to pass in a `Promise<string | string[]>` to do so. This promise can resolve whenever we know what to set and we await the promise before calling `glob` to get all file paths to delete.
Update changelog for 3.2.0
1fe3d0c to
6882d3e
Compare
2420de2 to
a37151b
Compare
a37151b to
2d30000
Compare
239808f to
baf5845
Compare
|
For the new package, this list needs to be considered: I think the new package is missing |
baf5845 to
cba358e
Compare
|
@timfish I copied over the LICENSE and README.md from the bundler-plugins-core package, since that's about the closest analogue to this refactor. Because it's semi-internal, I think some of the other "new SDK" steps don't really apply, but we can address them either before or after landing this, I don't really have a strong opinion about it. |
3ab9f64 to
413068a
Compare
| "@sentry/bundler-plugins": "file:../../../bundler-plugins/sentry-bundler-plugins-5.3.0.tgz", | ||
| "@sentry/bundler-plugin-core": "file:../../../bundler-plugin-core/sentry-bundler-plugin-core-5.3.0.tgz", | ||
| "@sentry/esbuild-plugin": "file:../../../esbuild-plugin/sentry-esbuild-plugin-5.3.0.tgz", | ||
| "@sentry/babel-plugin-component-annotate": "file:../../../babel-plugin-component-annotate/sentry-babel-plugin-component-annotate-5.3.0.tgz" |
There was a problem hiding this comment.
Wrong fixture tgz override paths
High Severity
Fixture pnpm.overrides still point at standalone-repo tarball paths under dev-packages (bundler-plugins, bundler-plugin-core, esbuild-plugin, etc.). After monorepo integration the packed plugin lives under packages/bundler-plugins, and separate core/esbuild-plugin directories are not present at those relative paths, so pnpm install in fixtures cannot resolve the intended local builds.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2449737. Configure here.
There was a problem hiding this comment.
This isn't quite right, but it does point out a pretty significant oversight. The bundler-plugin integration tests still use the older module names, which are not ported over in this merge.
Those need to be updated to use @sentry/bundler-plugins/<platform> instead of @sentry/<platform>-plugin, and built properly. I'll add another commit to this PR to do that.
There was a problem hiding this comment.
Hm, running the full bundler-plugins integration suite in CI is a bit more of a refactor. I think it's best for this PR to leave it in its current semi-broken state, get the code pulled in, and then fix it up to run properly and be integrated into CI. Attempting to do the full nx convention alignment right now would be pretty risky, I think, and while it's not in a great state, it's mostly harmless (doesn't break any other parts of the build, just isn't being tested fully).
@chargome @timfish What do you two think about this approach?
d032792 to
8e03cf7
Compare
8e03cf7 to
18f6ebe
Compare
| logger.warn('Running Sentry plugin from within a `node_modules` folder. Some features may not work.'); | ||
| } | ||
|
|
||
| const freeGlobalDependencyOnBuildArtifacts = createDependencyOnBuildArtifacts(); |
There was a problem hiding this comment.
Bug: A race condition in the Rollup plugin's watch mode can cause build artifacts to be deleted while they are still being uploaded from a previous build.
Severity: MEDIUM
Suggested Fix
The deleteArtifacts function should wait for all dependencies to be resolved, not just the global one. The per-upload dependency created within uploadSourcemaps should be properly awaited by the artifact deletion logic. This could involve making the dependency tracking mechanism aware of all active dependencies, not just the single global one that is freed prematurely in watch mode.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/bundler-plugins/src/rollup/index.ts#L82
Potential issue: In the Rollup plugin, a race condition can occur during watch mode when
sourcemap uploads and artifact deletion are enabled. A single global dependency is
created at initialization and freed in the `finally` block of every `writeBundle` hook.
This `finally` block then calls `sentryBuildPluginManager.deleteArtifacts()`, which
waits for dependencies to be freed. However, the sourcemap upload process, which runs
asynchronously, creates its own separate, per-upload dependency. If a new build starts
before the previous build's upload is complete, the global dependency can be freed
prematurely, allowing `deleteArtifacts()` to delete files that are still being used by
the ongoing upload from the previous build.
Also affects:
packages/bundler-plugins/src/rollup/index.ts:222~224
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 137f8fc. Configure here.
| @@ -0,0 +1,27 @@ | |||
| { | |||
| "name": "@sentry-internal/integration-tests-next", | |||
There was a problem hiding this comment.
Wrong integration test package name
Medium Severity
The new bundler-plugin integration package is named @sentry-internal/integration-tests-next, which matches the Next.js integration tests naming pattern, not bundler plugins. That mislabels the workspace package and can break Nx or CI targets that select tests by package name.
Reviewed by Cursor Bugbot for commit 137f8fc. Configure here.
| ); | ||
| return false; | ||
| } | ||
| if (setCommits.auto && setCommits.repo && setCommits) { |
There was a problem hiding this comment.
Bug: The validation for setCommits options incorrectly checks && setCommits instead of && setCommits.commit, leading to misleading warnings when commit is not provided.
Severity: LOW
Suggested Fix
Update the conditional statement to correctly check for the presence of setCommits.commit. Change if (setCommits.auto && setCommits.repo && setCommits) to if (setCommits.auto && setCommits.repo && setCommits.commit). This will ensure the warning only triggers when auto is combined with a complete manual configuration.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/bundler-plugins/src/core/options-mapping.ts#L195
Potential issue: The validation logic at line 195 checks `setCommits.auto &&
setCommits.repo && setCommits`. The final `&& setCommits` is redundant and the check for
`setCommits.commit` is missing. This causes a false-positive warning to be logged when a
user provides an incomplete manual configuration like `{ auto: true, repo: 'myRepo' }`
without `commit`. The warning message incorrectly states that it is ignoring `repo` and
`commit`, which is confusing as `commit` was never specified.
| const dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| COMPONENT_ANNOTATION_LOADER = path.resolve( | ||
| dirname, | ||
| typeof __dirname !== 'undefined' | ||
| ? 'component-annotation-transform.js' // CJS | ||
| : 'component-annotation-transform.mjs', // ESM | ||
| ); |
There was a problem hiding this comment.
Bug: The fallback logic for resolving COMPONENT_ANNOTATION_LOADER uses a relative path that breaks when the file is bundled into a shared chunk directory, such as _chunks/.
Severity: MEDIUM
Suggested Fix
The fallback logic should not rely on a relative path from the current file's location. Instead, it should construct a more robust path that correctly locates component-annotation-transform.js regardless of how bundlers like Rollup place the webpack4and5.ts file into shared chunks.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/bundler-plugins/src/webpack/webpack4and5.ts#L31-L37
Potential issue: A fallback mechanism to locate the `COMPONENT_ANNOTATION_LOADER` is
flawed. When the primary `require.resolve` fails (e.g., in a non-packaged source
environment), the fallback constructs a relative path from the current file's location.
However, because `webpack4and5.ts` is not a primary entry point, it can be bundled into
a shared `_chunks/` directory. In this scenario, the relative path resolution will fail
because the target file (`component-annotation-transform.js`) does not exist in the
`_chunks/` directory, breaking the loader functionality in development environments.


Note: the commit log in this PR is ridiculous, because it intentionally preserves the history of the sentry-javascript-bundler-plugins repo. Probably best to review locally. Attempting to do a rebase or squash merge will probably break GitHub and maybe the world 😅
Integrate the newly merged in
@sentry/bundler-pluginsproject to thesentry-javascript monorepo.
the
@sentry/bundler-pluginspackage, rather than the per-platformre-export shims.
The version has not been updated from its independent 5.x line, though
it may be a good idea to bump it up to align with the 10.x line that the
rest of the monorepo uses.