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: Replace @vueuse/head with @unhead/vue #804

Merged
merged 11 commits into from
Jun 21, 2023

Conversation

wattanx
Copy link
Collaborator

@wattanx wattanx commented Jun 11, 2023

πŸ”— Linked issue

Fixes: #803
Fixes: #792

❓ 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

Since @vueuse/head is scheduled to be deprecated, change to use @unhead/vue as in Nuxt 3.
Replacing @unhead/vue will fix some bugs.

πŸ“ Checklist

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

@wattanx wattanx marked this pull request as ready for review June 12, 2023 00:29
@danielroe danielroe requested a review from harlan-zw June 12, 2023 12:19
Comment on lines 27 to 29
nuxtApp.hooks.hook('render:before', () => { pauseDOMUpdates = true })
// wait for new page before unpausing dom updates
nuxtApp.hooks.hook('render:done', unpauseDom)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The hook does not exist in Nuxt 2, so it is changed from the original code of Nuxt 3.

original code is
https://github.com/nuxt/nuxt/blob/4ac4dfda15c5b92507334681c5d089cf6c5441c3/packages/nuxt/src/head/runtime/plugins/unhead.ts#L25-L29

Copy link
Contributor

@harlan-zw harlan-zw Jun 20, 2023

Choose a reason for hiding this comment

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

render:done is a server-side hook, so will never be called in this block (afaik?)

see https://v2.nuxt.com/docs/internals-glossary/internals-renderer

Copy link
Contributor

@harlan-zw harlan-zw Jun 20, 2023

Choose a reason for hiding this comment

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

I think we need to drop support for the pausing / unpausing feature.

This may introduce a potential flash of the title/classes for users and it may lead to unexpected ssr data, but not sure what else the solution would be without bridge implementing Suspense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for your kind review!
I remove pausing / unpausing feature.
Somehow I tried to support it in Nuxt 2, but it's difficult.

96eef65

@harlan-zw
Copy link
Contributor

harlan-zw commented Jun 12, 2023

Really nice work @wattan! Initial review looks good, I'll have another look with fresh eyes when I have a chance.

@wattanx wattanx requested a review from harlan-zw June 16, 2023 10:37
@harlan-zw
Copy link
Contributor

harlan-zw commented Jun 20, 2023

A couple of things I noticed while testing:

  • async updates not working (see comment below)

  • Vue is throwing warnings on the props

[Vue warn]: "style" is a reserved attribute and cannot be used as component prop.                                                                                                                                       2:18:45 pm

found in

---> <Meta>
       <Head>

I imagine this is also the case for the nuxt 3 components as well, we should ideally just use the exported components from unhead or vueuse/head, I'll need to do some work on them though.

Will keep testing and let you know if I spot anything else

@harlan-zw
Copy link
Contributor

harlan-zw commented Jun 20, 2023

An issue with this imeplementation is any promises within script setup won't be resolved before rendering the meta.

Consider the following code:

<script>
export default {
  async setup() {
    const data = await new Promise(resolve => {
      setTimeout(() => {
        resolve('test')
      }, 500)
    })
    useHead({
      title: data // SSR does not work, CSR will work
    })
  }
}
</script>

The SSR title would be undefined. I think this is a pretty poor DX.

The workaround is pretty simple though:

<script>
export default {
  setup() {
    const data = new Promise(resolve => {
      setTimeout(() => {
        resolve('test')
      }, 500)
    })
    useHead({
      title: data // SSR and CSR works, Unhead will resolve promises before rendering
    })
  }
}
</script>

I don't think we can solve for the first example without using Suspense and I don't think that's possible with nuxt/bridge.

Maybe just a documentation thing?

Copy link
Contributor

@harlan-zw harlan-zw left a comment

Choose a reason for hiding this comment

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

May need to remove pausing / unpausing feature

@danielroe
Copy link
Member

I don't think async script setup is possible at all with Nuxt 2, which may resolve this potential issue?

@wattanx
Copy link
Collaborator Author

wattanx commented Jun 20, 2023

I see that the title also supports Promise.
Since async setup is not available in Nuxt 2, it would be worth mentioning in the documentation.
(I have added the async title test.)

@wattanx wattanx requested a review from harlan-zw June 20, 2023 13:38
Copy link
Contributor

@harlan-zw harlan-zw left a comment

Choose a reason for hiding this comment

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

Alright in that case and with the updates, looks good.

Thanks again for the great work!

One minor optional thing to consider is adding the polyfills, this helps with any code which used the slightly different @vueuse/head API. May not be needed.

Example code:

import { polyfillAsVueUseHead, createHead } from '@unhead/vue'

const head = createHead()
// support the same api as @vueuse/head
polyfillAsVueUseHead(head)

@danielroe danielroe merged commit 3e3cf68 into nuxt:main Jun 21, 2023
@danielroe danielroe mentioned this pull request Sep 14, 2023
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.

Cannot override charset using useHead titleTemplate in useHead does not work correctly
3 participants