Skip to content
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: fix vue3 render warning loop #6010

Closed

Conversation

Rockergmail
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

fix issues:
#5916
#4743

why this happens:

  1. vue3 support the property ref in the second arg of h method, to support this feature: https://cn.vuejs.org/guide/essentials/template-refs.html
  2. when a template render with a undecalred variable, vue3 will call console.warn to print error stacks.
  3. sentry hijacked console, and add the message to breadcrumb with safeJoin
  4. safeJoin will transfer variables to string. The ref variable is a proxied object via Proxy. so, when called String(value), will trigger the getter of Proxy, and got undefined, so again vue3 will call console.warn to print error stacks
  5. just loop infinitely.

solutions:

  1. tried to test the variable is Proxy. No matter how you test, will trigger proxy getter or other handlers, so it loops.
  2. the only way can test is the stack information from vue3

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hi @Rockergmail and thanks for opening up this PR! Would be great to fix this long-standing bug.

I have a couple of questions. I'm not a Vue expert so it'd be great if you could tell me how the fix you're proposing works and why we need to modify the args that way. I'm not yet sure if we can fix this in the proposed way but I'll take it to the team.

EDIT: Seeing the CI results, our linter is not yet happy. Running yarn lint should tell you what's wrong

@Rockergmail
Copy link
Contributor Author

Rockergmail commented Oct 21, 2022

@Lms24 It seens I have explained why . And you can see the debug process here: #5916

let's take the demo for example: https://stackblitz.com/edit/vue-g6usgy?file=src%2Fmain.js

Or can you suggest me how can I explain to you in which way ? the vue3 render process ? vue3 render Mycomp.vue, when it comes to _ctx.test, it needs to get value of test. But due to vue3's mvvm, using Proxy to hijack varibles in the rendering context, so that it can collecte dependencies. When you try to modify the variable. It will trigger updates.

so when we try to get the value of test, will trigger getter handler of Proxified test, if the context get a test variable, it will return the value before collect dependence. if get no test variable, will call warn
image
(you can see the full function PublicInstanceProxyHandlers in packages/runtime-core/src/componentPublicInstance.ts of vue source code)

warn will print the stack of component's info.

component has a prop called ref which is a Proxified variable.

so, when we reach _consoleBreadcrumb of sentry/javascript, will call safeJoin to strinify the variable so that we can send it to the server and log it.

Because ref is a Proxified variable, when we call String(value) in the safeJoin, so it will call getter of ref. Due to the context has no ref variable, so the vue will warn again and agiain

As to why we need to modify the args that way:

  1. If we try to call String(value) to get the value, will loop. So we need to avoid doing this.
  2. If we try to test a variable is Proxy, there is no way. Because it will loop too.
  3. The only way to know the varibale is Proxy I can figure out is the print info from vue3 [Vue warn]: Property "test" was accessed during render but is not defined on instance. at <Anonymous ref=Ref< undefined > > at <App>. So I just modify the arg when we meet the ref=Ref<, which is for vue3 only.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for explaining. Sat down with the team and we think this is good to go. It's too bad that we can't more securely check for a Proxy but it seems like this really isn't possible and therefore I think your solution makes sense.

Tbh, we're overall not too happy with this small hack being in the framework-agnostic breadcrumbs integration. However, creating a Vue-specific integration (that's extending this integration) just for this one bug would also be a lot of work.

For future readers: This might however very well become an option in case we need to do more Vue-specific stuff in this integration.

It seems like our E2E tests are failing because our Auth token was not set for PRs coming from forks. Any chance that you created this branch before we merged #5986? Would you mind rebasing this to the current master?

packages/browser/src/integrations/breadcrumbs.ts Outdated Show resolved Hide resolved
@Lms24 Lms24 self-assigned this Oct 21, 2022
lgmt

Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@Rockergmail
Copy link
Contributor Author

It seems like our E2E tests are failing because our Auth token was not set for PRs coming from forks. Any chance that you created this branch before we merged #5986? Would you mind rebasing this to the current master?

sorry, i am not sure if i am clear what you mean.

  1. I should create this branch base on 80f586e ,right?
  2. rebase this to the current master? what is this and what is the current master? is this means my branch ? the current master means master branch of getsentry/sentry-javascript, right?

@Lms24

@Lms24
Copy link
Member

Lms24 commented Oct 21, 2022

Yes, I mean rebase your branch to master of getsentry/sentry-javascript.
Sorry for the confusion.

@Rockergmail
Copy link
Contributor Author

done. here: #6014 @Lms24

@Lms24
Copy link
Member

Lms24 commented Oct 24, 2022

Closing this in Favour of #6014

@Lms24 Lms24 closed this Oct 24, 2022
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.

2 participants