Why this change?

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.

This works for updates, but need to handle similar case in setInitialDOMProperties, which is called during initial render.

I think node.setAttribute is called during setInitialDOMProperties as well to set attributes:

setInitialDOMProperties -> setValueForProperty -> setAttribute/setAttributeNs

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.

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.

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

The snapshot below contains the server ids. Is this testing for preservation of ids from the server?

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.

Do we still need this assertion if we have the inline snapshot below?

The body of this function looks identical to the mount function. Should we try to share it across mount and update?

The update shouldn’t need to create the initializer (normally). Can we short cut this and avoid creating the unnecessary closure and array?

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.

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.

Ah, so the point is it not being seen as a hydration error but just as something to be patched 🤔

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.

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.

What does “don’t modify the output” mean? I’m just reading the value. Not modifying it.

This should be an invariant since our error minification pipeline doesn't support error constructors, yet.

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?

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__

I'd use ReactDOMServer.renderToString(...) to render this string so that you don't have to hard code the format.

Example:

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.

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.

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.

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:

That way we need less code overall.

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.

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.

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.

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.

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