Skip to content

Preact X useId#3583

Merged
JoviDeCroock merged 21 commits intomasterfrom
use-id-alternative
Sep 3, 2022
Merged

Preact X useId#3583
JoviDeCroock merged 21 commits intomasterfrom
use-id-alternative

Conversation

@JoviDeCroock
Copy link
Copy Markdown
Member

@JoviDeCroock JoviDeCroock commented Jun 23, 2022 •
edited
Loading

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
JoviDeCroock
JoviDeCroock commented Jun 23, 2022
View reviewed changes
JoviDeCroock
JoviDeCroock commented Jun 23, 2022
View reviewed changes
JoviDeCroock
JoviDeCroock commented Jun 23, 2022
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 23, 2022 •
edited
Loading

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
Copy Markdown

coveralls commented Jun 23, 2022 •
edited
Loading

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
Closed
@mjgerace
Copy link
Copy Markdown

mjgerace commented Aug 16, 2022

Any movement on this?

@developit
Copy link
Copy Markdown
Member

developit commented Aug 17, 2022

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

@itsbjoern
Copy link
Copy Markdown

itsbjoern commented Aug 17, 2022

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:

preact/hooks/src/index.js

Lines 36 to 39 in 186f5da

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
Copy Markdown
Member Author

JoviDeCroock commented Aug 17, 2022 •
edited
Loading

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 😅

@itsbjoern
Copy link
Copy Markdown

itsbjoern commented Aug 17, 2022

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

@itsbjoern
Copy link
Copy Markdown

itsbjoern commented Aug 17, 2022

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 ...

@JoviDeCroock JoviDeCroock mentioned this pull request Aug 17, 2022
Merged
marvinhagemeister
marvinhagemeister approved these changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member Author

JoviDeCroock commented Aug 23, 2022

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

@mjgerace
Copy link
Copy Markdown

mjgerace commented Aug 24, 2022

Is this okay to merge?

@Wallacy
Copy link
Copy Markdown

Wallacy commented Aug 30, 2022

Any news here?

JoviDeCroock
JoviDeCroock commented Sep 1, 2022
View reviewed changes
@JoviDeCroock JoviDeCroock changed the title Preact X useId alternative Preact X useId Sep 2, 2022
@marvinhagemeister
Copy link
Copy Markdown
Member

marvinhagemeister commented Sep 2, 2022

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
Copy Markdown
Contributor

PodaruDragos commented Sep 4, 2022

will there be a new release soon that integrates this ?

@marvinhagemeister
Copy link
Copy Markdown
Member

marvinhagemeister commented Sep 4, 2022

@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
Copy Markdown
Contributor

PodaruDragos commented Sep 5, 2022

@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 in to view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@andrewiggins andrewiggins Awaiting requested review from andrewiggins

@developit developit Awaiting requested review from developit

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

useId hook

9 participants

@JoviDeCroock @coveralls @mjgerace @developit @itsbjoern @Wallacy @marvinhagemeister @PodaruDragos @Aloento