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(nuxt): useId composable #23368

Merged
merged 36 commits into from
Jan 30, 2024
Merged

feat(nuxt): useId composable #23368

merged 36 commits into from
Jan 30, 2024

Conversation

TakNePoidet
Copy link
Contributor

@TakNePoidet TakNePoidet commented Sep 22, 2023

🔗 Linked issue

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

The useId composable creates a unique id.

📝 Checklist

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

@stackblitz
Copy link

stackblitz bot commented Sep 22, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@nuxt-studio
Copy link
Contributor

nuxt-studio bot commented Sep 22, 2023

Live Preview ready!

Name Edit Preview Latest Commit
Nuxt Docs Edit on Studio ↗︎ View Live Preview c8e26c9

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Nice idea! and relevant for #23255.

@TakNePoidet TakNePoidet changed the title feat(nuxt): useId composable feat(nuxt): getUniqueID utility Sep 25, 2023
@pi0
Copy link
Member

pi0 commented Sep 28, 2023

This is a nice idea however I think there are easy pitfalls cases if we base such broadly named composable on transform-based key generators for runtime extending limitations currently we have with key-based data fetching composables.

  • Using getUniqueID in a component that is rendered multiple times (like a UI text input which i suppose would be common for accessibility), it always uses one ID which is not probably what we want
image
  • Using getUniqueID() in reusable utilities (even in same SFC!) or externalized dependencies won't be effective and can lead to runtime issues (we could have runtime fallback and hydrate it somehow in that case)

  • getUniqueID() composable naming sounds it generates an increamental/unique name by each call which it won't

  • The getUniqueID can be possibly conflict with future nitro/uncrypto auto imported utils sharing common name (maybe also otherlibs or even vue-core sometime!)


Having these in mind, i think might makes sense to make sure we use a name that is clear it comes from nuxt and also it is an code-location-based static key. Something like getNuxtStaticID() (name is terrible just to give some idea)

Another idea might be to make the usage to be like a constant, instead of function calling like:

const id = '' /* nuxt-unique-id */

@TakNePoidet
Copy link
Contributor Author

You can try adding a local ID

  const nuxt = useNuxtApp()
  const localId = (nuxt.payload?.localIds?.[key] ?? 0) + 1
  nuxt.payload.localIds = nuxt.payload.localIds ?? {}
  nuxt.payload.localIds[key] = localId

Then repeatedly calling the component will not cause conficts

<script setup lang="ts">
import {getUniqueID} from '#imports'

withDefaults(defineProps<{ id?: string }>(), {
  id: () => getUniqueID()
});
</script>

<template>
  <label :for="id">Label</label>
  <br>
  <input type="text" :id="id" :placeholder="id"/>
</template>

image

@mayank99
Copy link

Given that the main use case for this utility is to generate unique IDREFs (for labeling inputs and creating other ARIA associations), I would expect that it is:

  • unique per component instance
  • stable after hydration

Here's a blog post that has some useful ideas and links: https://www.jovidecroock.com/blog/preact-use-id

@danielroe
Copy link
Member

danielroe commented Sep 29, 2023

Those are both good suggestions. A hybrid approach that gets a unique constant and uses it as prefix + an additional per-instance value seems good to me.

Marking as draft for now - do feel free to change that whenever it's good to go 🙏

@danielroe danielroe marked this pull request as draft September 29, 2023 11:00
Copy link
Contributor

github-actions bot commented Jan 8, 2024

🚀 Release triggered! You can now install nuxt@npm:nuxt-nightly@pr-23368

@danielroe danielroe added this to the 3.10 milestone Jan 10, 2024
@alex-eliot
Copy link

Given that the main use case for this utility is to generate unique IDREFs (for labeling inputs and creating other ARIA associations), I would expect that it is:

  • unique per component instance
  • stable after hydration

Here's a blog post that has some useful ideas and links: https://www.jovidecroock.com/blog/preact-use-id

I have given it a try locally (while also fixing some edge cases that are present in its current state), but I was unable to find a way to make Preact's solution work in SSR.

On the client, it is possible to locate a certain vnode's path by traversing up the tree until you reach the root, storing each node's index on its parent's children. But on the server, the instance.subTree property is null and after debugging I didn't find any information about the current vnode that could either help specify its path, or at least be used as a unique identifier that remains stable after hydration (instance.uid is not stable for example).

If we are able to locate the vnode's path in the tree on the server, or have some unique identifier to each instance that remains stable after hydration, then this composable could be used in any component without having to store the ids in the root element's attributes (it could go directly to the payload as a Record<string, number[]>), which means that its template would be completely irrelevant to whether you can use useId().

I'm not very well-versed in either Nuxt or Vue internals, so please tell me if I've missed something.

@manniL
Copy link
Member

manniL commented Jan 11, 2024

As a note - this is planned for Vue 3.5 to be supported natively. Source

@alex-eliot
Copy link

Oh that is very good news, thanks for sharing. Should be much more reliable this way as Vue handles the vnode tree.

Copy link
Member

atinux commented Jan 11, 2024

Exactly, I believe it should come from Vue core

@cernymatej
Copy link
Contributor

Does that mean that this isn't coming to Nuxt and will be entirely implemented in Vue core?

@danielroe danielroe requested a review from atinux January 29, 2024 23:12
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

I think we can go ahead and merge this for now, and swap across to the Vue native implementation when it comes (and iterate to fix any bugs). For now, there's nothing Nuxt-specific about this implementation other than the automatic provision of a key that is unique per file + line number - and all data is transferred from server -> client via data attributes.

@danielroe danielroe merged commit 658a0ff into nuxt:main Jan 30, 2024
36 checks passed
@github-actions github-actions bot mentioned this pull request Jan 30, 2024
@fabkho
Copy link

fabkho commented Feb 1, 2024

I get the following error when trying to use useId():
The requested module '*/node_modules/nuxt/dist/app/components/client-only.js?v=d16211f9' does not provide an export named 'clientOnlySymbol'

@danielroe
Copy link
Member

Try removing node_modules/.cache. It sounds like vite may not have updated its cache with the new version.

@fabkho
Copy link

fabkho commented Feb 1, 2024

@danielroe fixed it! Thanks a lot.

@DJafari
Copy link

DJafari commented Feb 2, 2024

image
are you sure this is ssr friendly ?

@fabkho
Copy link

fabkho commented Feb 3, 2024

@DJafari i am actually having the same problem.

@danielroe
Copy link
Member

If you have an issue, please open a new issue with a reproduction rather than commenting on this PR 🙏

@DJafari
Copy link

DJafari commented Feb 3, 2024

@danielroe Because I cannot recreate this problem in the new project, I am commenting here

but it's has a bug, in some case data-n-ids attribute doesnt add to element

i use useId in two component, in one component data-n-ids added, but in another, not

image

@manniL
Copy link
Member

manniL commented Feb 3, 2024

@DJafari no matter if you can reproduce it or not, commenting on a merged PR is not the place to report bugs. That should be an own issue. If you want reproduce the issue the problem might be part of your code then?

manniL pushed a commit that referenced this pull request Feb 18, 2024
Co-authored-by: Daniel Roe <daniel@roe.dev>
Co-authored-by: Sébastien Chopin <seb@nuxt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.