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

[Feature]: use vue 3.5 useId #1267

Closed
2 tasks
RayGuo-ergou opened this issue Sep 4, 2024 · 9 comments · Fixed by #1298
Closed
2 tasks

[Feature]: use vue 3.5 useId #1267

RayGuo-ergou opened this issue Sep 4, 2024 · 9 comments · Fixed by #1298
Assignees

Comments

@RayGuo-ergou
Copy link
Contributor

Describe the feature

Since vue released 3.5 and has a ssr friendly useId, would be better to switch to it.

I can think something like this for backward compatibility

import * as v from 'vue';

// @ts-expect-error vue 3.5
if (v.useId) {
  // @ts-expect-error vue 3.5
  return v.useId();
}

Additional information

  • I intend to submit a PR for this feature.
  • I have already implemented and/or tested this feature.
@Dino-Kupinic
Copy link

image
Is this still required?
I'm on Nuxt 3.13.1 and Vue 3.5.3
image

@RayGuo-ergou
Copy link
Contributor Author

RayGuo-ergou commented Sep 6, 2024

The ConfigProvider is still required unless migrated vue's useId in this lib.

The error you see is because muxt changed their useId to be an alias of vue's. And it could be undefined if not in an instance (e.g. toast) so you have to handle it yourself.

update: toast is fine, the id was not from radix-vue, thus IMO in most case it should be defined.

@zernonia
Copy link
Member

zernonia commented Sep 7, 2024

Correct @RayGuo-ergou . Your suggestion here should eliminate the need of passing idFunction in Nuxt 3.13.1 onwards, and hence avoiding the ts error above 😁

@RayGuo-ergou
Copy link
Contributor Author

Hi @zernonia should still handle the typescript error above tho as: https://github.com/vuejs/core/blob/b1430f250d6aef7f866d9a670895d83596119b42/packages/runtime-core/src/helpers/useId.ts#L8

Even tho I am not a big fan of ! but in this case I feel it's safe to do it as the places call useId should under a vue app instance.

But this should indeed resolve the hydration mismatch issue.

@Saeid-Za
Copy link
Contributor

Saeid-Za commented Sep 12, 2024

I was thinking to provide a default value for the config provider, at later stages we could mark useId as deprecated.
But wouldn't import * from "vue" disable tree shaking?
If so, could we sync import useId when version criteria is satisfied?
If I'm not mistaken, require is not supported in vite without a plugin.

@RayGuo-ergou
Copy link
Contributor Author

RayGuo-ergou commented Sep 12, 2024

But wouldn't import * from "vue" disable tree shaking?

image

Tried to build a simple project in both way, the file size seems identical. I am guessing due to vue's well structure export, vite can auto tree shaking the import.

If so, could we sync import useId when version criteria is satisfied?

I would suggest the nuxt way, make the useId as alias of vue's useId

@RayGuo-ergou
Copy link
Contributor Author

Did a quick test

// index.js

import * as util from './util'

util.foo()
// util.js

function foo() {
  console.log('foo')
}
function bar() {
  console.log('bar')
}

export {
  bar,
  foo,
}

run pnpx rollup index.js -f esm -o bundle.js

get

cat bundle.js
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: bundle.js
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ function foo() {
   2   │   console.log('foo');
   3   │ }
   4   │
   5   │ foo();

@donPuerto
Copy link

any update on how to resolve the issue?

@Dino-Kupinic
Copy link

any update on how to resolve the issue?

We are waiting for a PR, you can still use ConfigProvider like i mentioned above. If you get an error just <!-- @vue-ignore -->

<template>
  <!-- @vue-ignore -->
  <ConfigProvider :use-id="useIdFunction">
    <NuxtLayout>
      <NuxtPage />
      <Toaster />
    </NuxtLayout>
  </ConfigProvider>
</template>

@zernonia zernonia linked a pull request Sep 17, 2024 that will close this issue
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 a pull request may close this issue.

5 participants