-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Add useOpaqueIdentifier Hook #17322
Add useOpaqueIdentifier Hook #17322
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 815042f:
|
Details of bundled changes.Comparing: 7785a52...815042f react-dom
react-test-renderer
react-art
react-native-renderer
react-debug-tools
react-reconciler
ReactDOM: size: 🔺+16.5%, gzip: 🔺+15.6% Size changes (experimental) |
Details of bundled changes.Comparing: 7785a52...815042f react-dom
react-debug-tools
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! There are some cases missing but I'll chat to you about them in person
packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
Outdated
Show resolved
Hide resolved
@@ -110,7 +110,7 @@ export function getValueForAttribute( | |||
return expected === undefined ? undefined : null; | |||
} | |||
const value = node.getAttribute(name); | |||
if (value === '' + (expected: any)) { | |||
if (value === toString(expected)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if expected
is the opaque object, this would throw the string error. toString
doesn't convert the opaque object, which makes this just return value
, which is what it should be anyway.
@@ -18,7 +19,11 @@ export function setAttribute( | |||
attributeName: string, | |||
attributeValue: string | TrustedValue, | |||
) { | |||
node.setAttribute(attributeName, (attributeValue: any)); | |||
if (attributeValue.$$typeof === REACT_OPAQUE_OBJECT_TYPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for updates, but need to handle similar case in setInitialDOMProperties
, which is called during initial render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think node.setAttribute
is called during setInitialDOMProperties
as well to set attributes:
setInitialDOMProperties
-> setValueForProperty
-> setAttribute
/setAttributeNs
} | ||
|
||
container.innerHTML = | ||
'<div data-reactroot=""><div id="s_0">Child One</div><!--$!--><div>Fallback</div><!--/$--></div>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also assert that React didn't replace the original DOM node. I would grab a reference to the divs here, then after you hydrate, make sure they match the hydrated ones (you can use a ref for that part, or grab them out of the container again). Then do the same after the update.
Scheduler.unstable_flushAll(); | ||
jest.runAllTimers(); | ||
|
||
expect(container.innerHTML).toMatchInlineSnapshot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output is correct, but let's also test that there was only a single commit. You can do that with useEffect
:
const id = useUniqueID();
useEffect(() => {
Scheduler.unstable_yieldValue('Did commit');
});
I expect this test will fail; let's write the test first and then I can work with you on how to fix it.
root.render(<App />); | ||
}); | ||
|
||
expect(container.innerHTML).toMatchInlineSnapshot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you do expect(container).toMatchInlineSnapshot
pretty-format
will format the inner html as html (newlines and less escaping, closer to how you'd write html manually).
If you explicitly want innerHTML
, you can do expect(wrap(container.innerHTML)).toMatchInlineSnapshot
to at least avoid the inline escaping and wrapping quotes. wrap
comes from jest-snapshot-serializer-raw
and is already used in react-refresh
's tests
`); | ||
}); | ||
|
||
it('generates unique ids for client render on good server markup', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snapshot below contains the server ids. Is this testing for preservation of ids from the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. I agree. Snapshot tests aren't a good way of testing these things. Especially when there are so many of them. I'd drop all the snapshots and replace them with more specific assertions.
We don't provide any guarantees that this is actually the format we will use. In fact, these IDs are way too likely to conflict with a manual user provided ID. So we probably should change the format and we might have to change it again.
A change in the format will yield a lot of manual work to verify if it's correct or not.
The tests also don't actually test the important properties that are useful to preserve regardless of which format we use. I.e. that a client generated ID can't overlap with a server generated ID. Does any of your tests fail if you just change it to be react_
prefixed on both server and client? Or what if you accidentally make remove the auto-incrementing, does it fail any tests?
Instead of asserting against a snapshot, you could assert on the DOM that the IDs match. E.g. find the divs and spans and assert that node.getAttribute('aria-labelledby')
matches node.id
and that they don't match the other's IDs. Also, write a test where the client generated IDs are not suppose to match existing server generated
content. E.g. by on the client adding a client-rendered pair. That's all that really matter. Not the exact format.
expect(Scheduler).toFlushAndYield([]); | ||
jest.runAllTimers(); | ||
|
||
expect(divNode).toEqual(ref.current); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this assertion if we have the inline snapshot below?
if (getIsHydrating()) { | ||
return { | ||
$$typeof: REACT_OPAQUE_OBJECT_TYPE, | ||
_setId: () => { | ||
setId(() => 'c_' + (clientId++).toString()); | ||
}, | ||
toString() { | ||
throw Error( | ||
'The object passed back from useUniqueID is meant to be passed through to ' + | ||
'attributes only. Do not directly modify the output.', | ||
); | ||
}, | ||
}; | ||
} else { | ||
return 'c_' + (clientId++).toString(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body of this function looks identical to the mount function. Should we try to share it across mount and update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update shouldn’t need to create the initializer (normally). Can we short cut this and avoid creating the unnecessary closure and array?
return { | ||
$$typeof: REACT_OPAQUE_OBJECT_TYPE, | ||
_setId: () => { | ||
setId(() => 'c_' + (clientId++).toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the same problem as most of the "userspace" solutions: this ID is unstable and depends on commit order which, especially in combination with Suspense and partial hydration, is not necessarily the same as tree order.
One thing library authors often asked for is a way of getting the "tree position" of the element, which is better and relatively stable, but still not perfect; it'll probably work better for hydration and will be globally unique, but might get reused if a component is swapped by another for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with this solution it doesn't matter whether the counter changes between server and client since react is able to patch it up properly during hydration which wasn't possible before.
This hook is exclusively for IDREF attributes. It's not a generic, unique ID of your component instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so the point is it not being seen as a hydration error but just as something to be patched 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least this is how I understood it. There's an argument to be made about hydration warnings for attributes in the first place (e.g. <button disabled={!process.browser} />
but IDREFs seem like a safe place to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not as simple as just patching. Partially because we don’t actually compare any props in hydration. But mostly because if you do it the naive way you may patch up one side but not the other side when partial hydration is being used.
This impl enforces that we force hydration of the things that needs patching on both sides.
We should have a test for that.
How about It’s not necessarily an ID in its implementation. It’s just some way of referencing one node in two places. The connection to I’ve thought about having this in React Native as a way to pass a native box around. So that I can pass a reference to a native View instance to native APIs. Eg getting a handle to a native View instance from another view’s View Manager on Native threads. Unlike useRef, this kind of ref’s current value would only be accessible from native code. That’s very similar to how you can think about this use case too. It’s a bit strange since you might expect to pass the target side like |
toString() { | ||
throw Error( | ||
'The object passed back from useUniqueID is meant to be passed through to ' + | ||
'attributes only. Do not directly modify the output.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does “don’t modify the output” mean? I’m just reading the value. Not modifying it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an invariant
since our error minification pipeline doesn't support error constructors, yet.
) { | ||
node.setAttribute(attributeName, (attributeValue: any)); | ||
if ( | ||
typeof attributeValue === 'object' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably React’s hottest path that we’re making slower (and not inlinable). Is this the best place we can do this? Where does TrustedValue do its check?
@@ -260,6 +264,17 @@ function useDeferredValue<T>(value: T, config: TimeoutConfig | null | void): T { | |||
return value; | |||
} | |||
|
|||
function useOpaqueReference(): string | IdObject { | |||
const hook = nextHook(); | |||
const value = hook === null ? 'FAKE_ID' : hook.memoizedState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name "FAKE_ID" doesn't say much and is kind of confusing if you do end up seeing it. What's Fake about it? Can I search for it? It also doesn't apply as a string when in React Native for example. Let's just leave it as undefined
if it's not there.
I believe this can also cause an error if you inspect a hydrated component since toString
and valueOf
will throw. We should probably detect if it's a REACT_OPAQUE_OBJECT_TYPE
and do something special. Perhaps just leaving it as undefined
as well.
You can add tests to: https://github.com/facebook/react/tree/9fe1031244903e442de179821f1d383a9f2a59f2/packages/react-debug-tools/src/__tests__
const container = document.createElement('div'); | ||
|
||
container.innerHTML = | ||
'<div data-reactroot=""><div id="s_0">Child One</div><!--$!--><div>Fallback</div><!--/$--></div>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use ReactDOMServer.renderToString(...)
to render this string so that you don't have to hard code the format.
Example:
react/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
Line 1049 in 9fe1031
let finalHTML = ReactDOMServer.renderToString(<App />); |
@@ -60,7 +86,8 @@ if (enableTrustedTypesIntegration && typeof trustedTypes !== 'undefined') { | |||
trustedTypes.isScript(value) || | |||
trustedTypes.isScriptURL(value) || | |||
/* TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204 */ | |||
(trustedTypes.isURL && trustedTypes.isURL(value))) | |||
(trustedTypes.isURL && trustedTypes.isURL(value)) || | |||
value.$$typeof === REACT_OPAQUE_OBJECT_TYPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. This will be toString:ed by the DOM and throw an error anyway. No need to add a special case, is there? Just let it fail below.
value !== null && | ||
typeof value === 'object' && | ||
value.$$typeof === REACT_OPAQUE_OBJECT_TYPE && | ||
typeof value._setId === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need this extra check. You've asserted the type. Just cast it through let obj: IdObject = (value: any)
below to satisfy Flow.
value._setId(); | ||
return ''; | ||
} else { | ||
return toStringOrTrustedType(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point to collocate these was so that you could use the same type check to quickly determine if this is not an object which is the common case. However, by calling this second function, you have to check the type again in the other function. Leading to two checks instead of one, for the common case.
You should inline this here so that you can avoid extra type checks. However, the bigger question is why you need two functions at all.
@@ -44,14 +45,39 @@ export opaque type TrustedValue: {toString(): string, valueOf(): string} = { | |||
valueOf(): string, | |||
}; | |||
|
|||
export function evaluateToStringOrTrustedType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be its own function? Can't it just be the same as toStringOrTrustedType
?
toStringOrTrustedType
is called in two other places.
This one seems like you might have missed: https://github.com/facebook/react/pull/17322/files#diff-214d5116dd8b5a7d184cdf5b2160f94cR183
Seems like it should also trigger this path.
The other one is here but I think that's actually already the wrong place to do this and it shouldn't toString this at all since the initial rendering doesn't:
toStringOrTrustedType(nextHtml), |
But it doesn't hurt that we check for
REACT_OPAQUE_OBJECT_TYPE
. Even though the existing call is a bug in the first place.
That way we need less code overall.
@@ -110,7 +114,7 @@ export function getValueForAttribute( | |||
return expected === undefined ? undefined : null; | |||
} | |||
const value = node.getAttribute(name); | |||
if (value === '' + (expected: any)) { | |||
if (value === toStringOrTrustedType(getToStringValue(expected))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call site seems like it breaks with trusted types already.
But this fix doesn't seem right. Why call getToStringValue
? Why would you need to ignore function
values or symbol
? getAttribute
should never return those types.
toStringOrTrustedType
also doesn't seem like the right fix neither.
For trusted types, the server generated string won't match the object on the client, so this wouldn't end up matching. Causing a hydration error. This will need to dig into the trusted type and inspect the value to see if it's equivalent.
For REACT_OPAQUE_OBJECT_TYPE
the returned object will never match. So that will also be a hydration error.
It seems to me that this needs to just perform some more custom matching logic instead of trying to use the toStringOrTrustedType
helper.
@@ -197,6 +198,9 @@ if (__DEV__) { | |||
if (didWarnInvalidHydration) { | |||
return; | |||
} | |||
if (clientValue && clientValue.$$typeof === REACT_OPAQUE_OBJECT_TYPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong place to do this check. If we get here it's because we already thought that there was an error. If you do the check in getValueForAttribute
instead, then we don't have to treat it as an error in the first place.
const [id, setId] = mountState(() => { | ||
if (getIsHydrating()) { | ||
return { | ||
$$typeof: REACT_OPAQUE_OBJECT_TYPE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than its creation, this object is only ever referenced in ReactDOM
. That's a good thing because on other platforms like the React Native use case, this object will look very different and have a very different signature.
To do that, though, we need to make the creation of this object something platform specific. Let's make a HostConfig method to create this object and only implement it in ReactDOM.
For React Native, we should make that method throw since this feature is not yet supported there. Later we could add some specific implementation.
You probably need to pass setId
in as an argument to the HostConfig
method.
}, | ||
}; | ||
} else { | ||
return 'c_' + (clientId++).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also move to ReactDOM
since it's specific to how an "Opaque Reference" is constructed in the DOM.
Also, don't call toString()
. Rely on + ''
casting instead like we do elsewhere. .toString()
becomes a dynamic look up which can be monkey patched by Number.prototype
. It's more bytes and less efficient.
Actually, scratch that, let's use toString(36)
instead to save some bytes in memory on the ID. On the client it doesn't matter as much but nice on the server so might as well use it for consistency.
</div> | ||
</div> | ||
`); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a similar test where Child One is in a Suspense boundary that is still suspended. I.e. if we're only partially hydrated. Then we do an update that shows the second Child. This should suspend and wait for the suspense boundary to resolve before letting the update through.
It should not leave them inconsistent.
That test would ensure that we don't forget about this case and try to implement the "patching strategy approach" in the future. That test is really the main driver for this particular implementation strategy so we should ensure we test for it.
https://github.com/facebook/react/pull/17322/files#r354445736
We currently use unique IDs in a lot of places. Examples are:
*
<label for="ID">
*
aria-labelledby
This can cause some issues:
1. If we server side render and then hydrate, this could cause an hydration ID mismatch
2. If we server side render one part of the page and client side render another part of the page, the ID for one part could be different than the ID for another part even though they are supposed to be the same
3. If we conditionally render something with an ID , this might also cause an ID mismatch because the ID will be different on other parts of the page
This PR creates a new hook
useOpaqueIdentifier
that generates a different unique ID based on whether the hook was called on the server or client. If the hook is called during hydration, it generates an opaque object that will rerender the hook so that the IDs match.