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

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-julienhuang-julien commented Sep 22, 2022

Copy link
Copy Markdown
Member

🔗 Linked issue

linked to #6165 (comment)
resolvesnuxt/nuxt#15014, resolvesnuxt/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

Copy link
Copy Markdown

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

@netlify

netlifyBot commented Sep 22, 2022

Copy link
Copy Markdown

Deploy Preview for nuxt3-docs ready!

NameLink
🔨 Latest commit0e9ccd3
🔍 Latest deploy loghttps://app.netlify.com/sites/nuxt3-docs/deploys/6331877876654c0008d564fd
😎 Deploy Previewhttps://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.

@danielroedanielroe 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

@danielroedanielroe 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
MemberAuthor

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

@danielroedanielroe 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 threadtest/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
MemberAuthor

i forgot to set .client on some test components

@pi0pi0 changed the title fix(nuxt): remove Fragment from createClientOnlyfix(nuxt): remove fragment from createClientOnlySep 26, 2022
@huang-julien

huang-julien commented Sep 26, 2022

Copy link
Copy Markdown
MemberAuthor

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 danielroeSeptember 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 danielroeSeptember 26, 2022 10:23
@atinux
atinux merged commit e6ca07b into nuxt:mainOct 3, 2022
@danielroedanielroe mentioned this pull request Oct 9, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3.xbugSomething isn't working🔨 p3-minor-bugPriority 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

@huang-julien@antfu@danielroe@atinux