useId hook #3402
Closed
useId hook #3402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +316 B (0%) Total Size: 38.6 kB
ℹ️ View Unchanged
|
JoviDeCroock
commented
Jan 9, 2022
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-final
03_update10th1k_x16
duration
usedJSHeapSize
07_create10k
duration
usedJSHeapSize
filter_list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many_updates
duration
usedJSHeapSize
text_update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
|
JoviDeCroock
requested review from
developit,
andrewiggins and
marvinhagemeister
JoviDeCroock
commented
Jan 22, 2022
src/create-root.js
Outdated
| @@ -11,6 +11,8 @@ import { mount } from './diff/mount'; | |||
| import { patch } from './diff/patch'; | |||
| import { createInternal } from './tree'; | |||
|
|
|||
| let rootId = 0; | |||
There was a problem hiding this comment.
We might want to introduce some randomness here in case there are multiple completely separate Preact installations running, i.e. 2 widgets as those will be two roots with a resetting _domDepth
There was a problem hiding this comment.
FYI: In devtools I'm using this to sort of namespace the ids of different preact installations
// Create an integer-based namespace to avoid clashing ids caused by
// multiple connected renderers
const namespace = Math.floor(Math.random() * 2 ** 32);
There was a problem hiding this comment.
Randomness would break hydration with SSR though, no?
There was a problem hiding this comment.
Right, good point about the randomness!
hooks/src/index.js
Outdated
| const state = getHookState(currentIndex++, 11); | ||
| if (!state._id) { | ||
| currentIdCounter++; | ||
| state._id = (currentInternal._rootId + currentInternal._domDepth + currentIdCounter).toString(32) |
There was a problem hiding this comment.
Do you know how collision resistant this is?
Also, do we need to worry about preact users using a simple ID (single digit) themselves?
There was a problem hiding this comment.
Prefixed with _P now
JoviDeCroock
commented
Mar 20, 2022
| '_P' + | ||
| currentInternal._rootId + | ||
| (currentInternal._parent._children.indexOf(currentInternal) + | ||
| currentInternal._depth + |
There was a problem hiding this comment.
_domDepth might be more reliable here as sometimes people wrap their ssr with a wrapper for the routing, ... we could also just remove it as the global id counter should take care of it either way
We could even just do
'_P' +
currentInternal._rootId + currentIdCounterThe only thing that currently bothers me is the fact that this could be annoying when we talk about plugins, maybe we allow for custom prefixes to help them out?
Something like
export function useId(customPrefix) {
const state = getHookState(currentIndex++, 11);
if (!state._id) {
currentIdCounter++;
state._id =
customPrefix || '_P' +
currentInternal._rootId +
currentIdCounter
}
return state._id;
}
JoviDeCroock
commented
Apr 8, 2022
hooks/src/index.js
Outdated
|
|
||
| state._id = | ||
| '_P' + | ||
| currentInternal._rootId + |
There was a problem hiding this comment.
hmm rts should set this as well then
|
Does this approach support |
|
@dvoytenko it does yes as it does not take any global counters, it will rely on the state of the subtree which in suspense is a stable thing to rely on. A combination of a rootId, the position of the node in the parents children and the amount of useId invocations for a node. In React this implementation is a lot more complex as they bitshift forks of the tree I assume for concurrency issues |
|
Is it closed because there's an alternative PR? |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements #3373
Currently this hook leverages a combination of the component
_vnodeId(ensures uniqueness across switching subtrees and roots), the vnode depth (ensures uniqueness within a subtree) and the order of the hook.EDIT1:
Replacing the usage of
_vnodeIdand_depthwith a root-identifier and the dom-depth because in SSR currently there are a lot of cases where a subtree isn't rendered until we're on the client due to it being personal information this could skew_vnodeIdin our current implementation. Same goes for_depthif SSR has an additional or missing FunctionalComponent the_depthwill be erroneous, we avoid missmatches during hydration so_domDepthis far more reliable.This adds tracking for a rootId and _domDepth
TODO: uniqueness across siblings, this could prove troublesome if siblings are fenced with Fragments
In this example both Child's will see a _domDepth of 0 and a parentPosition of 0 so this could produce the same id which is erroneous.
EDIT2: replaced the counter that counted the amount of
useIdinvocations scoped to a vnode to be a global counter which will bypass the issue outlined above.EDIT3: the uniqueness across siblings has been caught by looking at the index the child has inside of the parentInternal._children, we could now scope the counter to be by vnode to avoid big numbers if we incorporate something like a
_domDepthunless we want to assume_depthto be consistentThis is currently blocked by changes needed to RenderToString