-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
New hook: useInstanceId #19071
New hook: useInstanceId #19071
Changes from 2 commits
5e3e830
618abd8
bf48985
727ecd8
b889563
76b2f9d
1cdcd04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useMemo, useEffect } from '@wordpress/element'; | ||
|
||
/** | ||
* Array to keep track of allocated ids. | ||
*/ | ||
const allocatedIds = []; | ||
|
||
/** | ||
* Find an unallocated id. | ||
*/ | ||
function findId() { | ||
let id = allocatedIds.findIndex( ( allocated ) => ! allocated ); | ||
|
||
if ( id === -1 ) { | ||
id = allocatedIds.length; | ||
} | ||
|
||
// Allocated the id. | ||
allocatedIds[ id ] = true; | ||
|
||
return id; | ||
} | ||
|
||
/** | ||
* Free an allocated id. | ||
* | ||
* @param {number} id Id to free. | ||
*/ | ||
function freeId( id ) { | ||
delete allocatedIds[ id ]; | ||
} | ||
|
||
/** | ||
* Provides a unique instance ID. | ||
*/ | ||
export default function useInstanceId() { | ||
// Take advantage of useMemo to get the same id throughout the life of a | ||
// component. | ||
const id = useMemo( findId, [] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this might be a use-case for
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It’s ok if the id changes. With useRef, wouldn’t findId be called on every render? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, right, didn't pay attention to that part 😬 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, you're right 👍 At a glance, I don't see a way around that, so maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, using https://reactjs.org/docs/hooks-faq.html?no-cache=1#how-to-create-expensive-objects-lazily There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, doesn't do much unless it happens often. It could be a problem with async mode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's see. If need be, we can create an alternative to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, a |
||
// Free up the id when the comonent unmounts. This must depend on `id` since | ||
// useMemo is not guaranteed to return the same id throughout the life of | ||
// the component. | ||
useEffect( () => () => freeId( id ), [ id ] ); | ||
return id; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { create, act } from 'react-test-renderer'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import useInstanceId from '../'; | ||
|
||
describe( 'useInstanceId', () => { | ||
const TestComponent = () => { | ||
return useInstanceId(); | ||
}; | ||
|
||
it( 'should manage ids', async () => { | ||
let test0; | ||
|
||
await act( async () => { | ||
test0 = create( <TestComponent /> ); | ||
} ); | ||
|
||
expect( test0.toJSON() ).toBe( '0' ); | ||
|
||
let test1; | ||
|
||
await act( async () => { | ||
test1 = create( <TestComponent /> ); | ||
} ); | ||
|
||
expect( test1.toJSON() ).toBe( '1' ); | ||
|
||
test0.unmount(); | ||
|
||
let test2; | ||
|
||
await act( async () => { | ||
test2 = create( <TestComponent /> ); | ||
} ); | ||
|
||
expect( test2.toJSON() ).toBe( '0' ); | ||
|
||
let test3; | ||
|
||
await act( async () => { | ||
test3 = create( <TestComponent /> ); | ||
} ); | ||
|
||
expect( test3.toJSON() ).toBe( '2' ); | ||
|
||
test1.unmount(); | ||
test2.unmount(); | ||
} ); | ||
} ); |
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 slightly less performant than just increment the id, any concern about that?
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'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.
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.
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.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.
Use a clean up stack:
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.
That works too. Either you have a pool of allocated ids or a pool of free ids.
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 of the stack is to avoid the linear lookup.
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.
Sounds good, I’ll adjust it. Thanks!