Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.

fix(nuxt): remove fragment from createClientOnly - #7774

Merged
atinux merged 11 commits into
nuxt:mainfrom
huang-julien:fix/client-only
Oct 3, 2022
Merged

fix(nuxt): remove fragment from createClientOnly#7774
atinux merged 11 commits into
nuxt:mainfrom
huang-julien:fix/client-only

Conversation

@huang-julien

@huang-julien huang-julien commented Sep 22, 2022

Copy link
Copy Markdown
Member

🔗 Linked issue

linked to #6165 (comment)
resolves nuxt/nuxt#15014, resolves nuxt/nuxt#15039

❓ Type of change

  • 📖 Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Hi 👋 👋 This PR remove the Fragment workaround which prevent using v-show on .client components even if it has a single root node.

There is additionnal changes in the setup part where we return the render function.

Previously in the linked PR, there's was a Oldchildren is null issue if we returned directly the setup result as a render function.
Using the h() function to return the VNode was working only for some components (based on @danielroe 's #6165 (comment)).

However the component fails to render or update its render silently if it only has a root tag with a string and a variable such as :

<template>
  <div>
      I'm a string children {{ count }}. I will be mounted but will still render a simple div
  </div>
</template>

The workaround was to use a Fragment but it leads to nuxt/nuxt#15014 issue .

This is fixed by verifying the result of the render function children which can be string|array|null|object and rendering the component using createElementVNode when res.children is null or a string

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codesandbox

codesandbox Bot commented Sep 22, 2022

Copy link
Copy Markdown

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify

netlify Bot commented Sep 22, 2022

Copy link
Copy Markdown

Deploy Preview for nuxt3-docs ready!

Name Link
🔨 Latest commit 0e9ccd3
🔍 Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/6331877876654c0008d564fd
😎 Deploy Preview https://deploy-preview-7774--nuxt3-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.

@danielroe danielroe added bug Something isn't working 🔨 p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Sep 23, 2022

@danielroe danielroe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! ❤️

This seems like a good fix to me. Is there a way we can add a test for the different kinds of client-only components? Maybe a page in the fixture with a variety of types and we can use createPage to assert that all content is correctly displayed?

@huang-julien
huang-julien marked this pull request as draft September 23, 2022 13:58
@huang-julien

huang-julien commented Sep 23, 2022

Copy link
Copy Markdown
Member Author

I think this should now cover most of the issues we can run into.

let me know if i missed something :)

@huang-julien
huang-julien marked this pull request as ready for review September 23, 2022 15:07

@danielroe danielroe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great, and I'm really happy with the test suite - thank you 😊

Comment thread test/basic.test.ts Outdated
@huang-julien
huang-julien marked this pull request as draft September 26, 2022 09:38
@huang-julien

Copy link
Copy Markdown
Member Author

i forgot to set .client on some test components

@pi0 pi0 changed the title fix(nuxt): remove Fragment from createClientOnly fix(nuxt): remove fragment from createClientOnly Sep 26, 2022
@huang-julien

huang-julien commented Sep 26, 2022

Copy link
Copy Markdown
Member Author

updated #7774 (comment)

  • fixed missing .client in test components name.
  • Use createElementVNode instead of createElementBlock. Components without any state or script are not rendered using createElementBlock.
  • test V show directive reactivity using a ref

@huang-julien
huang-julien requested review from antfu and danielroe and removed request for antfu and danielroe September 26, 2022 10:22
@huang-julien
huang-julien marked this pull request as ready for review September 26, 2022 10:23
@huang-julien
huang-julien requested review from antfu and danielroe and removed request for antfu and danielroe September 26, 2022 10:23
@atinux
atinux merged commit e6ca07b into nuxt:main Oct 3, 2022
@danielroe danielroe mentioned this pull request Oct 9, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3.x bug Something isn't working 🔨 p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3.0 rc.11 - Classes not being merged from parent to child component (in client-only components) client only components don't respect v-show

4 participants