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

feat(nuxt): parse html to treeshake client-only components #7527

Merged
merged 10 commits into from Oct 10, 2022

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

resolves nuxt/nuxt#14896

❓ 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

This switches from regexp -> parse5 (fast HTML parsing) to remove client only component nodes. It's more stable and also allows us to preserve fallback slots, for example.

The blocker for this PR is currently unjs/jiti#84; jiti is having trouble transforming parse5, blocking testing.

πŸ“ Checklist

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

@danielroe danielroe added bug Something isn't working enhancement New feature or request πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Sep 14, 2022
@danielroe danielroe self-assigned this Sep 14, 2022
@netlify
Copy link

netlify bot commented Sep 14, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 83bf655
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/633051aa1e602c0009eca831

@pi0
Copy link
Member

pi0 commented Sep 15, 2022

I will look into jiti issue. Isn't there an easy way to fix the regex issue? Parse5, even if super fast is stil parsing the strings. We might use a hybrid method (like mlly) to improve precision.

@danielroe
Copy link
Member Author

Indeed I could fix that particular issue with regexp but I feel parsing the HTML is more reliable. For example, there was always the caveat of nested client-only components.

At some point you reinvent the parser. And I believe parse5 will soon be exposed to us by vite, so we can even benefit from vite caching ast (iirc): vitejs/vite#9678

@pi0
Copy link
Member

pi0 commented Sep 15, 2022

Yes, I'm aware that vite itself is using parse5 and it is a really good decision.

But here we are (re) parsing the same template code into AST in a plugin for a particular optimization that benefits only some components. Full parsing makes the built-time overall slower.

If there are finally more edge cases that we cannot handle with regex only, same as mlly we can use a two phase detection with quick regex matcher and precision AST (or Tokenized) verifier when we really are going to make a change to the code.

It would be nice if first could resolve the linked issue via regex and keep working on this PR as a general enhancenment.

@pi0
Copy link
Member

pi0 commented Sep 19, 2022

@danielroe Anyway we can workaround nuxt/nuxt#14896 regex issue?

@danielroe
Copy link
Member Author

~> #7659

@danielroe
Copy link
Member Author

Updating this PR just to keep it in sync. You were right to be careful; parse5 doesn't handle arbitrary self-closing elements. I suppose we could use an xml parser, but a big appeal of it for me was aligning with vite.

@danielroe
Copy link
Member Author

πŸš€ Switched to ultrahtml, a 1.75kB library with zero dependencies and deliberate support for parsing Vue.

@danielroe danielroe marked this pull request as ready for review September 25, 2022 12:59
@danielroe danielroe removed the pending label Sep 25, 2022
@danielroe danielroe requested a review from pi0 September 25, 2022 12:59
@danielroe danielroe mentioned this pull request Oct 9, 2022
@danielroe danielroe merged commit 26b1c9c into main Oct 10, 2022
@danielroe danielroe deleted the fix/parse-client-html branch October 10, 2022 15:48
@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 enhancement New feature or request πŸ”¨ 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.

<ClientOnly> with <template #fallback> always results in hydration error in RC10
3 participants