fix(utils): Prevent iterating over VueViewModel #8981
Conversation
There was a problem hiding this comment.
Hi @Duncanxyz thanks for opening this PR! I'm not too happy about including framework-specific edge cases in a general package but I think there's no way around it. Also, we did this before as we can see in the code (and in a nother place for Vue iirc).
I have an important question though: Is there a chance that other objects than VueViewModel might unnecessarily be normalized here? Can other Vue objects have the same _isVue flag? In case yes, maybe it's still fine to normalize them but we should probably change the string to something more generic like "[VueObject]". If this is not possible, let's leave it as is.
|
|
||
| // React's SyntheticEvent thingy | ||
| if (isSyntheticEvent(value)) { | ||
| return '[SyntheticEvent]'; |
There was a problem hiding this comment.
well I guess there's precedence for handling this stuff like this in a general package of ours 😅
There was a problem hiding this comment.
the tricky thing is that some people end up just using @sentry/browser even when using a framework like React or Vue.
but I think we should re-examine this in v8 and see if we can somehow inject in framework specific processing.
I agree, this is not an elegant solution. Perhaps in the future, we can implement it through a plugin approach, where additional handling can be injected when referencing framework-specific packages (e.g., @sentry/vue).
I took the naming reference from @sentry/vue. The My experience with naming might not be extensive enough, so do you have any suggestions regarding this?
I have tested both Vue 2 and Vue 3, but the issue only occurs in Vue 2. After your reminder, I finded that Vue 3 had similar issues and handling before (#4743, #5916, #6014), which essentially were caused by accessing illegal properties of the Vue instance during the rendering process, and warning in Vue's DEV mode. I will make changes to the code later to ensure compatibility with both Vue 2 and Vue 3. |
There was a problem hiding this comment.
Thank you for explaining!
Perhaps in the future, we can implement it through a plugin approach
Maybe, yes - good idea! But for now, this is fine.
However, the current issue usually occurs only on Vue instance objects.
My experience with naming might not be extensive enough, so do you have any suggestions regarding this?
No, after reading your explanations, I think it's fine to keep it as-is.
I will make changes to the code later to ensure compatibility with both Vue 2 and Vue 3.
So just to confirm before we ✅ , this PR currently fixes an issue that's only present in Vue 2 and Vue 3 is not affected by this PR? Then we're good :)
…eadcrumb 1. Preserve the original behavior of console. 2. Centralize special handling for Vue in the utils module.
I made a mistake; Vue 3 also has issues, just with different colors 😅. The code I just submitted includes compatibility optimizations for both Vue 2 and Vue 3. vue2: vue3: |
|
|
||
| // React's SyntheticEvent thingy | ||
| if (isSyntheticEvent(value)) { | ||
| return '[SyntheticEvent]'; |
There was a problem hiding this comment.
the tricky thing is that some people end up just using @sentry/browser even when using a framework like React or Vue.
but I think we should re-examine this in v8 and see if we can somehow inject in framework specific processing.
|
@Duncanxyz this will go out in version 7.69.0 in the next couple of hours. Thanks again! |
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).close #8980