Uh oh!
There was an error while loading. Please reload this page.
fix(vue): Update Vue trackComponents list to match components with or without <> - #13543
Conversation
…or without `<>` Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| if (!(currentCompo.startsWith('<') && currentCompo.endsWith('>'))) { | ||
| currentCompo = `<${currentCompo}>`; | ||
| } | ||
| return formattedName === currentCompo; |
There was a problem hiding this comment.
formattedName from formatComponentName() always return a name with < >.
There was a problem hiding this comment.
Oh, it can also be <Component> at Component.vue.
Lms24
left a comment
There was a problem hiding this comment.
Hey @Zen-cronic thanks for opening this PR!
The change looks good to me but I had a slightly more size-efficient solution.
Could you add a test as well? We already have a Vue 3 test E2E test application. I was thinking that we could set a trackComponents array option in the Sentry init call and adjust the pageload test that it also includes a ui.vue.* span for a component that matches one of the array entries.
If you don't have time, just let me know, then I'll add the test.
| let currentCompo = compo; | ||
| if (!(currentCompo.startsWith('<') && currentCompo.endsWith('>'))) { | ||
| currentCompo = `<${currentCompo}>`; | ||
| } | ||
| return formattedName === currentCompo; |
There was a problem hiding this comment.
I think we can replace this with a more bundle size efficient version:
| letcurrentCompo=compo; | |
| if(!(currentCompo.startsWith('<')&¤tCompo.endsWith('>'))){ | |
| currentCompo=`<${currentCompo}>`; | |
| } | |
| returnformattedName===currentCompo; | |
| returncompo.replace(/^<(.*)>$/,'$1')===formattedName |
There was a problem hiding this comment.
Ah in this case we don't even need the block anymore but could inline it to the some call. So feel free to do that as well :)
There was a problem hiding this comment.
hey, I've taken your approach and found that formattedName must also be replaced:
functionfindTrackComponent(trackComponents: string[],formattedName: string): boolean{functionextractComponentName(name: string): string{returnname.replace(/^<(.*)>(?:.*)?$/,"$1");}constisMatched=trackComponents.some(compo=>{returnextractComponentName(formattedName)===extractComponentName(compo)});returnisMatched;}The regex is needed for both the user provided component and the formatted component.
- User provided: either
<App>orApp - Formatted: either
<App>or<App> at App.vue.
What do you think?
There was a problem hiding this comment.
Good observation, you're right! Also, I didn't think about the <App> at App.vue case.
There's one unfortunate side effect of this regex though: It can lead to polynomial build up when evaluating an input. Given that we run extractComponentName on user input I'd rather not risk the (still unlikely but possible) chance of some kind of ReDoS attack (I used this ReDoS checker to test it)
I played around with the regex to make it safer and keep it linear and arrived at this one: /^<([^\s]*)>( at [^\s]*)?$
I think this should still cover all component names but would appreciate it if you could double check it as well :)
Also, since this logic is getting a little bit more involved now, could you add a couple of unit test for findTrackComponent? It's fine to export the function just for testing purposes btw. Thanks!
(I don't want to overburden you with tests btw, I just want to keep stuff like this covered and understandable for everyone. So if you don't have the time, just let me know. We appreciate your hard work either way!)
There was a problem hiding this comment.
Noted, unit and e2e tests added.
Zen-cronic
commented
Sep 4, 2024
Just read your comments and review, I'll take them up now. Thanks! |
Lms24
left a comment
There was a problem hiding this comment.
Sorry, I realized I approved the PR yesterday on accident. Please don't mind the change request, I'm just putting it here so that we don't merge the PR accidentally before the extract name logic is resolved.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| * | ||
| */ | ||
| function extractComponentName(name: string): string { | ||
| return name.replace(/^<([^\s]*)>(?: at [^\s]*)?$/, '$1'); |
There was a problem hiding this comment.
Based on what you suggested @Lms24, just an added non-capture group. The ReDoS Checker passes.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| 'sentry.op': 'ui.vue.mount', | ||
| 'sentry.origin': 'auto.ui.vue', | ||
| }, | ||
| description: 'Vue <<HomeView>>', |
There was a problem hiding this comment.
Just making sure, are the double brackets intended? They're formatted here.
There was a problem hiding this comment.
Good observation! I don't think they're intended. Let's remove them.
Lms24
left a comment
There was a problem hiding this comment.
Thanks for making the changes! We're almost there :)
| }); | ||
| }); | ||
| test('sends a lifecycle span for the tracked HomeView component - with `<>`', async ({ page }) => { |
There was a problem hiding this comment.
This is a bit of an unfortunate and very intransparent limitation of our e2e test setup but we shouldn't navigate to the same route in more than one e2e test. The reason is that we execute them in parallel in CI and the waitForTransaction call from one test might actually resolve for a transaction in another test, depending on the condition in the callback.
To avoid this confusion, we generally create a new route for each test. Suggestion: Either we create new routes for the two new tests or we integrate them in already existing tests and simply check for the spans there. I'll let you make the call, I'm fine with either option.
Also, feel free to test both kinds trackComponent notations in one route/test :)
There was a problem hiding this comment.
hey, thanks for the info. i decided to go with creating a new route, and grouped the different notations together in a single test.
Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
Group trackComponent tests together: 1. With <> 2. Without <> 3. Not tracked Signed-off-by: Kaung Zin Hein <kaungzinhein113@gmail.com>
| <h1>Demonstrating Component Tracking</h1> | ||
| <ComponentOneView /> | ||
| <ComponentTwoView /> | ||
| </template> |
There was a problem hiding this comment.
Inspired by the sveltekit-2-svelte-5 test setup.
Lms24
left a comment
There was a problem hiding this comment.
Thanks a lot @Zen-cronic, looks great to me now!
By the way, the entire JS SDK team really appreciates your contributions! @mydea sent you a message with a small thank you gift on LinkedIn a couple of days ago, in case you missed it. If this message somehow got lost, please feel free to drop me or @mydea an email (email in GH bio) :)
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13543 Co-authored-by: Lms24 <8420481+Lms24@users.noreply.github.com>
Resolves#13510
Tests to be added.
yarn lint) & (yarn test).