Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(core): Only inject release into entrypoints per default #166

Merged
merged 7 commits into from
Jan 19, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Jan 19, 2023

Fixes #163

Turns out our approach for injecting the release value sucks in case you want to preserve modules while transpiling because we'll create a throwing require("\0sentry-release-injector") statement since \0sentry-release-injector is only virtual and you can't "preserve" a virtual file.

This PR is fixing this by slightly changing how the releaseInjectionTargets option works. By default we'll inject the release into all entry points now (as opposed to all modules) - without any import magic - just by injecting code. This may have bundle size impact but users can still control how this works by providing a function to releaseInjectionTargets.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

I like this approach. Seems to be a lot simpler than what we had before. I guess this is also kinda "behaviorally" breaking, so let's maybe add a migration note as well?
lmk when this is ready to ✅

@@ -253,15 +219,22 @@ const unplugin = createUnplugin<Options>((options, unpluginMetaContext) => {
* @param id Always the absolute (fully resolved) path to the module.
* @returns transformed code + source map
*/
transform(code, id) {
async transform(code, id) {
logger.debug('Called "transform":', { id });

// The MagicString library allows us to generate sourcemaps for the changes we make to the user code.
const ms = new MagicString(code);

// Appending instead of prepending has less probability of mucking with user's source maps.
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still valid/necessary?

Copy link
Member

Choose a reason for hiding this comment

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

(referring to line 228)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup thanks! :)

@lforst lforst self-assigned this Jan 19, 2023
@lforst lforst marked this pull request as ready for review January 19, 2023 10:44
@lforst
Copy link
Member Author

lforst commented Jan 19, 2023

@Lms24 should be ready for review!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice, let's go!

@lforst lforst enabled auto-merge (squash) January 19, 2023 10:51
@lforst lforst merged commit 6ca81b8 into main Jan 19, 2023
@lforst lforst deleted the lforst-inject-release-into-entrypoints branch January 19, 2023 10:56
@pdmshrestha
Copy link

pdmshrestha commented Feb 22, 2023

After upgrading from 0.3.0 to 0.4.0, it seems that release is not injected property.

"@sentry/tracing": "^7.38.0",
"@sentry/vite-plugin": "^0.4.0",
"@sentry/vue": "^7.38.0",

Didn't have any issue on v0.3.0.

I am using vite to bundle the assets.

import { sentryVitePlugin } from "@sentry/vite-plugin";

X-posted in #178

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.

esbuild-plugin: Error: Cannot find module 'sentry-release-injector'
3 participants