-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Inject init code via relative path #8135
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
Conversation
@@ -79,6 +79,8 @@ export default function wrappingLoader( | |||
vercelCronsConfig, | |||
} = 'getOptions' in this ? this.getOptions() : this.query; | |||
|
|||
this.resourcePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this?
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge after @timfish confirms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my alternative PR here:
#8142
templateCode = `import "${sentryConfigFilePath}";`.concat(templateCode); | ||
if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { | ||
const sentryConfigFileImportPath = path | ||
.relative(this.resourcePath, sentryConfigFilePath) // Get path relative to current module because webpack can't handle absolute paths on windows: https://github.com/getsentry/sentry-javascript/issues/8133 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be path.dirname(this.resourcePath)
otherwise the relative path goes up one level too far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that seems to be the most important thing!
@@ -202,8 +202,13 @@ export default function wrappingLoader( | |||
templateCode = templateCode.replace(/__COMPONENT_TYPE__/g, 'Unknown'); | |||
} | |||
|
|||
if (sentryConfigFilePath) { | |||
templateCode = `import "${sentryConfigFilePath}";`.concat(templateCode); | |||
if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the check here for isAbsolute
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think webpack really defines what the value behind this looks like. Usually it is an absolute path but I think it could also be some virtual module and stuff and we can't create relative imports for virtual modules so it is just a check for the cases we can actually handle. Does this make sense?
if (sentryConfigFilePath && path.isAbsolute(this.resourcePath)) { | ||
const sentryConfigFileImportPath = path | ||
.relative(this.resourcePath, sentryConfigFilePath) // Get path relative to current module because webpack can't handle absolute paths on windows: https://github.com/getsentry/sentry-javascript/issues/8133 | ||
.replace(/\.[^/.]+$/, ''); // Remove the file extension from the import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to replace every \
with /
. Import paths cannot use backslashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that in my testing, the file extension didn't seem to matter!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for pointing that out. Was just messing around here and yeah doesn't seem to fix anything haha
Potentially fixes #8133