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

fix: Stub instance of the same component #1979

Merged

Conversation

freakzlike
Copy link
Collaborator

@freakzlike freakzlike commented Feb 15, 2023

Fixes #1162

I'm not really happy with the wording, because there is a lot of double negation. However, the code does the job

@netlify
Copy link

netlify bot commented Feb 15, 2023

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit bb080b9
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/63f3babea95f6e0008001370
😎 Deploy Preview https://deploy-preview-1979--vue-test-utils-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@freakzlike freakzlike requested review from cexbrayat and xanf February 15, 2023 16:52
@xanf
Copy link
Collaborator

xanf commented Feb 15, 2023

Thank you @freakzlike, I'll go ahead and test this across real codebase right today :)

Copy link
Member

@cexbrayat cexbrayat 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 taking a stab at this @freakzlike
Code looks good, even if I agree it's hard to understand all the negations of negations

export const addToDoNotStubComponents = (
type: ConcreteComponent,
onlyRoot?: boolean
) => doNotStubComponents.set(type, !!onlyRoot)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could have two distinct functions, with different names to make it easier to understand?
like addToDoNotStubComponentsButOnlyInRoot? (I admit it's not a great name ^^)

@xanf
Copy link
Collaborator

xanf commented Feb 16, 2023

I'm iterating on this one, and while I'm working on suggestion, would like to share my thoughts:

  • we are already tracking "root" instances for auto-destroy purposes, so what I'm thinking might be is drop addToDoNotStubComponents logic for root but instead when making stub logic check it from registry of "root" instances (and move trackInstance logic and reigstry to separate f ile

@freakzlike
Copy link
Collaborator Author

I'm iterating on this one, and while I'm working on suggestion, would like to share my thoughts:

  • we are already tracking "root" instances for auto-destroy purposes, so what I'm thinking might be is drop addToDoNotStubComponents logic for root but instead when making stub logic check it from registry of "root" instances (and move trackInstance logic and reigstry to separate f ile

You are right. It's only used for the root components and it might be a better way

@freakzlike
Copy link
Collaborator Author

I tried to retrieve or check for the root component without the need of rootComponents, but didn't manage to do so for all cases. The logic has not changed now, but it's a bit easier to read I think

@freakzlike freakzlike requested a review from cexbrayat February 18, 2023 08:22
@lmiller1990
Copy link
Member

Need a rebase, since we just released the renderToString PR.

Glad @xanf is running this on a real code base - will wait back to hear from him before merging this one up.

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

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

LGTM

This needs to be rebased, as some logic moved to src/createInstance now that renderToString landed (sorry)

@lmiller1990
Copy link
Member

Shall we merge or did we want more testing from @xanf? Happy to move forward as-is.

@xanf
Copy link
Collaborator

xanf commented Feb 21, 2023

@freakzlike @lmiller1990 I'm going to finish testing no later than tomorrow, sorry for delay - our codebase uses v-once, and I need to use vuejs with this PR vuejs/core#7730 merged to test it, cause compiler blows up

@lmiller1990
Copy link
Member

I think we should merge and release this one. I'll give it another 24h then ship.

@lmiller1990 lmiller1990 merged commit eba8d30 into vuejs:main Mar 9, 2023
@freakzlike freakzlike deleted the fix/1162/stub-recursive-component branch March 31, 2023 08:00
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.

shallowMount doesn't stub instances of the same component
4 participants