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

Preact X useId #3583

Merged
merged 21 commits into from Sep 3, 2022
Merged

Preact X useId #3583

merged 21 commits into from Sep 3, 2022

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Jun 23, 2022

Currently to the thought of a prefix tree this would be an alternative to useId. This tries to follow our component tree and has a mask variable.

Resolves #3373

Mount

Every time we encounter a component a long the way we will set vnode._mask to the global counter we are following, this means that every node that can generate an id will have a unique mask. When we invoke useId we will look at the localIdCounter (reset for every component we see) this with the combination of our prefix will ensure that we generate a consistent id even if we are invoking multiple useId calls.

Mounting a new subtree

When we see a new subtree we will look at the parentMask and count from there, this however means that we could cause conflicts, ideally we would need to keep incrementing mask. If we can find a way to reset the mask counter for SSR environments we would bypass this.

Notes

  • We can optimize subsequent client-side renders by using a counter that increments after hydration but atm I don't think we have a good way to track this

DEMO: https://codesandbox.io/s/crazy-platform-omyl5l?file=/src/hooks.js

@JoviDeCroock JoviDeCroock marked this pull request as draft June 23, 2022 15:08
hooks/src/index.js Outdated Show resolved Hide resolved
hooks/src/index.js Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jun 23, 2022

Size Change: +463 B (0%)

Total Size: 52.8 kB

Filename Size Change
compat/dist/compat.js 3.78 kB +7 B (0%)
compat/dist/compat.module.js 3.72 kB +11 B (0%)
compat/dist/compat.umd.js 3.85 kB +5 B (0%)
hooks/dist/hooks.js 1.55 kB +151 B (9%) 🔍
hooks/dist/hooks.module.js 1.57 kB +141 B (8%) 🔍
hooks/dist/hooks.umd.js 1.63 kB +148 B (9%) 🔍
ℹ️ View Unchanged
Filename Size Change
debug/dist/debug.js 3.01 kB 0 B
debug/dist/debug.module.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.09 kB 0 B
devtools/dist/devtools.js 231 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
dist/preact.js 4.01 kB 0 B
dist/preact.min.js 4.04 kB 0 B
dist/preact.min.module.js 4.04 kB 0 B
dist/preact.min.umd.js 4.07 kB 0 B
dist/preact.module.js 4.03 kB 0 B
dist/preact.umd.js 4.08 kB 0 B
jsx-runtime/dist/jsxRuntime.js 358 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 324 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 439 B 0 B
test-utils/dist/testUtils.js 442 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 526 B 0 B

compressed-size-action

@coveralls
Copy link

coveralls commented Jun 23, 2022

Coverage Status

Coverage increased (+0.005%) to 99.527% when pulling 1aad6d8 on use-id-alternative into 1427d58 on master.

@JoviDeCroock JoviDeCroock marked this pull request as ready for review July 3, 2022 08:03
@JoviDeCroock JoviDeCroock mentioned this pull request Jul 4, 2022
@mjgerace
Copy link

Any movement on this?

@developit
Copy link
Member

@mjgerace I believe Jovi has a PR Open in render-to-string to add the necessary links for this.

@BFriedrichs
Copy link

Hi just to let you know I checked out this and the PR on render-to-string (preactjs/preact-render-to-string#226)

In my setup this line breaks:

const position =
vnode._parent && vnode._parent._children
? vnode._parent._children.indexOf(vnode)
: 0;

This is because indexOf is not defined on vnode._parent._children, I investigated this and it seems like _children is a rendered preact object rather than a list. In my case I fixed it by just making the line:

const position = vnode._parent &&
    vnode._parent._children &&
    vnode._parent._children !== vnode &&
    vnode._parent._children.indexOf

I realise this is quite hacky (and probably doesn't do what I actually want it to) but at least it doesn't crash anymore. I have basically no knowledge of this project and just needed stuff to work.

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Aug 17, 2022

You mean it breaks on the client or during SSR, if it breaks on the client that might be expected I haven't gone through all cases yet.

As mentioned this is in no way a done thing 😅

@BFriedrichs
Copy link

It breaks during the render call of render-to-string.

@BFriedrichs
Copy link

The more I think about it, given the scuffed setup I'm using it is very possible that the fault is actually somewhere on my end ...

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

This is great! 💯

@JoviDeCroock
Copy link
Member Author

@developit had an alternative that had a better byte-size impact

@mjgerace
Copy link

Is this okay to merge?

@Wallacy
Copy link

Wallacy commented Aug 30, 2022

Any news here?

hooks/src/index.js Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock changed the title Preact X useId alternative Preact X useId Sep 2, 2022
@marvinhagemeister
Copy link
Member

Let's merge this 👍

@JoviDeCroock JoviDeCroock merged commit 803dbb5 into master Sep 3, 2022
@JoviDeCroock JoviDeCroock deleted the use-id-alternative branch September 3, 2022 07:22
@PodaruDragos
Copy link
Contributor

will there be a new release soon that integrates this ?

@marvinhagemeister
Copy link
Member

@PodaruDragos I definitely share your excitement about useId! We merged this PR just two days ago and it was the weekend. We maintainers need some time to recharge and relax too :)

@PodaruDragos
Copy link
Contributor

@marvinhagemeister my apologies for sounding demanding, yeah I am really waiting for this for a long time.
Thanks for this feature and thanks for all the good work you guys are doing on this project.

@Aloento

This comment was marked as duplicate.

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.

useId hook
9 participants