New hook: useInstanceId

[ellatrix]

Description

Edit: I've now created a better alternative at #19091.

This hook is a replacement for withInstanceId.

The implementation is a bit different. Instead of just continuously incrementing the id (number), it keeps track of the allocated ids. When the component unmounts, the id is free to be used again.

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[youknowriad]

This is slightly less performant than just increment the id, any concern about that?

[ellatrix]

I'm not concerned as this only executes when a component mounts. Incrementing ids is simple, but it has no end. The highest number possible here is the amount of mounted components per page.

[ellatrix]

withInstanceId has counters for each component. I don't think we can do that with hooks. The counter would be per page load. So the number could potentially reach much higher than before. That's why I decided to reuse ids for unmounted components.

[epiqueras]

Use a clean up stack:

let nextId = 0;
const freedIds = [];
function findId() {
	if ( freedIds.length ) {
		return freedIds.pop();
	}
	return nextId++;
}
function freeId( id ) {
	freedIds.push( id );
}

[ellatrix]

That works too. Either you have a pool of allocated ids or a pool of free ids.

[epiqueras]

The point of the stack is to avoid the linear lookup.

[ellatrix]

Sounds good, I’ll adjust it. Thanks!

[aduth]

Seems like this might be a use-case for useRef instead?

The returned object will persist for the full lifetime of the component.

It’s handy for keeping any mutable value around similar to how you’d use instance fields in classes.

https://reactjs.org/docs/hooks-reference.html#useref

i.e.

const id = useRef( findId() );
// ...
return id.current;

Unclear whether it would also make it okay to avoid passing id as the dependency of useEffect when used this way, if we have a stronger guarantee that the value won't change.

[epiqueras]

id itself would never change. id.current would. But we should still pass id, to align with the planned linting rules.

[ellatrix]

It’s ok if the id changes. With useRef, wouldn’t findId be called on every render?

[epiqueras]

Oh, right, didn't pay attention to that part 😬

[aduth]

With useRef, wouldn’t findId be called on every render?

Yes, you're right 👍 At a glance, I don't see a way around that, so maybe useMemo is the way to go.

[epiqueras]

Actually, using useMemo for this is not recommended, because it's not always respected:

https://reactjs.org/docs/hooks-faq.html?no-cache=1#how-to-create-expensive-objects-lazily

[ellatrix]

I know, but as I said before, it's fine if it's not respected. Then a now id is given and the previous id is freed up. It doesn't do any harm, right?

[epiqueras]

Yeah, doesn't do much unless it happens often.

It could be a problem with async mode.

[ellatrix]

Let's see. If need be, we can create an alternative to useMemo that never gets called again.

[epiqueras]

Sounds good, a useStrictMemo hook 😆

[aduth]

Build failure appears to be caused by this line:

gutenberg/packages/e2e-tests/specs/editor/various/embedding.test.js

Line 254 in b2ef0f5

const postUrl = await page.$eval( '#inspector-text-control-0', ( el ) => el.value );

I think it might have to change, since we can't rely on a specific ID anymore (it probably shouldn't have been written this way in the first place, basing it off a dynamic ID).

It might work as...

const postUrl = await page.$eval( '[id^="inspector-text-control-"]', ( el ) => el.value );

[aduth]

I have a feeling that if these IDs are now shared in a global scope, our snapshots will become increasingly unreliable, because they can be impacted by changes in the rendering behavior of unrelated components.

It's as similar issue to what I expressed at #14995 (comment) .

[ellatrix]

@aduth Right, they are shared globally. I haven't found a way around this with hooks, unfortunately. There's no longer a wrapper around the functions to store the counter.

[ellatrix]

@aduth I don't understand fully how the snapshot would become unreliable though. If everything is rendered in the same order within one test and the tests are isolated, then it shouldn't be a problem?

[ellatrix]

I now created #19091, which is much more like withInstanceId. I'll leave this PR open so both options can be considered, but I now much more prefer #19091 since it doesn't affect the tests at all and the behaviour is exactly the same.

[ellatrix]

Looks like we're going for #19091. Closing this.