Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(nuxt): Configure sentry in external config #12681
feat(nuxt): Configure sentry in external config #12681
Changes from 1 commit
5feabda
7c329d4
e1f1ade
a0079a8
78ec3a0
77a741c
31c1158
5f93aa6
b24f39d
2f5097f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
m: this should be both esm and cjs
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 actually I'd double check this too to see if ESM works well on the server 😅
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.
TODO: Will we have to do the same as for solid here, where types for a submodule export have to be in the root? 😬 no need to adjust this in this PR, but have a look at it in a follow up, and maybe sync with @andreiborza on this :)
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.
l: why elect for the inline function
pathToPosix
overThere 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.
True 😅 I had two functions first and moved
pathToPosix
inside the other one.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.
m: Let's extract
<script setup>
into a constant, and add a comment about why we are looking for this.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.
l: I think doing
string.replace
is a bit easier to understand thenindexOf
+ constructing a new output viaslice
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.
m: Can we indicate that this comes from sentry? Same with
console.error
below.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.
l: Would be useful to leave a comment about why this we have a difference between vue init and nuxt init here.
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.
l: Can we double check to see if this works with CJS? How does this get transpiled? We currently have
build/module/module.cjs
emitted.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.
For the module, I am using the nuxt module builder which transpiles the files exactly how they should be imported in Nuxt. The
module.cjs
looks like this: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.
m: we should add some notes that this is coming from sentry and we encountered this error while trying to add imports.
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.
m: It seems like this PR has some server-side logic with the
addServerPlugin
and the server side sdk being exposed.To ease reviewing, could we remove these changes from this PR and add it later when we look at the server-side stuff?
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.