Skip to content

Conversation

@lforst
Copy link

@lforst lforst commented Aug 22, 2022

This PR rethinks the entire approach of how we're injecting the global release variable. We're moving from prepending the injector code in entry files to injecting an import statement into all user files.

It sounds a bit dubious that we're injecting appending the same import to all user files but it has its reasons: With the way unplugin and bundlers work, we do not always have the information we need to only inject code at the top of an entry file. The transform and transformInclude hooks don't have any information as to whether a file is an entry file - so they need context from another hook for that. The only location where we can determine if a file is an entry file is the resolveId hook - sadly in this hook, we do not have an guaranteed absolute path of the file we're looking at, which would require us to do a heuristic (using process.cwd()) which isn't bulletproof.

@lforst lforst mentioned this pull request Aug 22, 2022
29 tasks
@lforst lforst marked this pull request as ready for review August 22, 2022 14:37
@lforst lforst requested review from Lms24 and vladanpaunovic August 22, 2022 14:37
Comment on lines +32 to +65
* ```text
* index.js (user file)
* ┌───────────────────┐ ┌─────────────────────────────────────────────────┐
* │ │ │ import { myFunction } from "./my-library.js"; │
* │ sentry-unplugin │ │ │
* │ │ │ const myResult = myFunction(); │
* └---------│---------┘ │ export { myResult }; │
* │ │ │
* │ injects │ // injected by sentry-unplugin │
* ├───────────────────► import "sentry-release-injector"; ─────────────────────┐
* │ └─────────────────────────────────────────────────┘ │
* │ │
* │ │
* │ my-library.js (user file) │
* │ ┌─────────────────────────────────────────────────┐ │
* │ │ export function myFunction() { │ │
* │ │ return "Hello world!"; │ │
* │ │ } │ │
* │ │ │ │
* │ injects │ // injected by sentry-unplugin │ │
* └───────────────────► import "sentry-release-injector"; ─────────────────────┤
* └─────────────────────────────────────────────────┘ │
* │
* │
* sentry-release-injector │
* ┌──────────────────────────────────┐ │
* │ │ is resolved │
* │ global.SENTRY_RELEASE = { ... } │ by unplugin │
* │ // + a little more logic │<─────────────────────┘
* │ │ (only once)
* └──────────────────────────────────┘
* ```
*
* Source maps upload:
Copy link
Member

Choose a reason for hiding this comment

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

I love this, explains it very well!

@lforst lforst merged commit fe67ecd into main Aug 22, 2022
@lforst lforst deleted the lforst-import-approach branch August 22, 2022 15:50
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.

3 participants