From 596b81bf9dd2a6b8c69c3ef8b82fc77de7ec84b1 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Fri, 21 Oct 2022 23:30:10 +0200 Subject: [PATCH 1/8] Fix useId not unique on shared component parent + DOM --- hooks/src/index.js | 42 ++++++++++++++++++-------------- hooks/src/internal.d.ts | 4 --- hooks/test/browser/useId.test.js | 40 ++++++++++++++++++++++++------ mangle.json | 3 +-- 4 files changed, 58 insertions(+), 31 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 9f5d098139..72511b0e21 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -1,4 +1,4 @@ -import { options } from 'preact'; +import { Fragment, options } from 'preact'; /** @type {number} */ let currentIndex; @@ -27,22 +27,6 @@ const RAF_TIMEOUT = 100; let prevRaf; options._diff = vnode => { - if ( - typeof vnode.type === 'function' && - !vnode._mask && - // Ignore root Fragment node - vnode._parent !== null - ) { - vnode._mask = - (vnode._parent && vnode._parent._mask ? vnode._parent._mask : '') + - (vnode._parent && vnode._parent._children - ? vnode._parent._children.indexOf(vnode) - : 0); - } else if (!vnode._mask) { - vnode._mask = - vnode._parent && vnode._parent._mask ? vnode._parent._mask : ''; - } - currentComponent = null; if (oldBeforeDiff) oldBeforeDiff(vnode); }; @@ -395,7 +379,29 @@ function hash(s) { export function useId() { const state = getHookState(currentIndex++, 11); if (!state._value) { - state._value = 'P' + hash(currentComponent._vnode._mask) + currentIndex; + // Traverse the tree upwards and count the components until we reach + // the root node. + let root = currentComponent._vnode; + let parent = root._parent; + let i = 0; + while (parent !== null) { + if (parent.type !== Fragment && typeof parent.type === 'function') { + i++; + } + + root = parent; + parent = parent._parent; + } + + // Attach the id usage array to the root node and resize it to fit + let ids = root._ids || (root._ids = [0]); + while (ids.length < i) { + ids.push(0); + } + + // Increase the current component depth pointer + ids[i - 1]++; + state._value = 'P' + hash(ids.join('')) + currentIndex; } return state._value; diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index 58a2741384..a555e4ec49 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -38,10 +38,6 @@ export interface Component extends PreactComponent { __hooks?: ComponentHooks; } -export interface VNode extends PreactVNode { - _mask?: string; -} - export type HookState = | EffectHookState | MemoHookState diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index 232dffcf0a..b7d25ebaea 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -55,12 +55,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); }); @@ -83,12 +83,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); }); @@ -121,13 +121,13 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); set(true); rerender(); expect(scratch.innerHTML).to.equal( - '
hh
' + '
hh
' ); }); @@ -350,6 +350,32 @@ describe('useId', () => { } render(, scratch); - expect(scratch.innerHTML).to.equal('

P481

P476951

'); + expect(scratch.innerHTML).to.equal('

P481

P15671

'); + }); + + it('should skip over HTML', () => { + const ids = []; + + function Foo() { + const id = useId(); + ids.push(id); + return

{id}

; + } + + function App() { + return ( +
+ + + + + + +
+ ); + } + + render(, scratch); + expect(ids).to.deep.equal(['P491', 'P501']); }); }); diff --git a/mangle.json b/mangle.json index 6633035754..e51b759649 100644 --- a/mangle.json +++ b/mangle.json @@ -39,7 +39,6 @@ "$_depth": "__b", "$_nextDom": "__d", "$_dirty": "__d", - "$_mask": "__m", "$_detachOnNextRender": "__b", "$_force": "__e", "$_nextState": "__s", @@ -81,4 +80,4 @@ "$_isSuspended": "__i" } } -} \ No newline at end of file +} From 8579923902c8f1676863ddf240bbc9c308ea0a5d Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Sat, 22 Oct 2022 22:18:13 +0200 Subject: [PATCH 2/8] Add test for same renderToString ids per root --- hooks/test/browser/useId.test.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index b7d25ebaea..48c9fafac6 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -378,4 +378,31 @@ describe('useId', () => { render(, scratch); expect(ids).to.deep.equal(['P491', 'P501']); }); + + it('should reset for each renderToString roots', () => { + const ids = []; + + function Foo() { + const id = useId(); + ids.push(id); + return

{id}

; + } + + function App() { + return ( +
+ + + + + + +
+ ); + } + + const res1 = rts(); + const res2 = rts(); + expect(res1).to.equal(res2); + }); }); From e094e27bb555dc6a76592dea42b39483f7d1122c Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 17:54:11 +0200 Subject: [PATCH 3/8] Add conditional components test case --- hooks/test/browser/useId.test.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index 48c9fafac6..4d96b40e45 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -405,4 +405,29 @@ describe('useId', () => { const res2 = rts(); expect(res1).to.equal(res2); }); + + it('should work with conditional components', () => { + function Foo() { + const id = useId(); + return

{id}

; + } + function Bar() { + const id = useId(); + return

{id}

; + } + + let update; + function App() { + const [v, setV] = useState(false); + update = setV; + return
{!v ? : }
; + } + + render(, scratch); + const first = scratch.innerHTML; + + update(v => !v); + rerender(); + expect(first).not.to.equal(scratch.innerHTML); + }); }); From 46ddfeb5fffc13f402b79545e5713f1612628446 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 18:20:15 +0200 Subject: [PATCH 4/8] Drop useId hashing as it's not needed anymore --- hooks/src/index.js | 11 +---------- hooks/test/browser/useId.test.js | 16 ++++++++-------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 72511b0e21..931f9d7ad7 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -367,15 +367,6 @@ export function useErrorBoundary(cb) { ]; } -function hash(s) { - let h = 0, - i = s.length; - while (i > 0) { - h = ((h << 5) - h + s.charCodeAt(--i)) | 0; - } - return h; -} - export function useId() { const state = getHookState(currentIndex++, 11); if (!state._value) { @@ -401,7 +392,7 @@ export function useId() { // Increase the current component depth pointer ids[i - 1]++; - state._value = 'P' + hash(ids.join('')) + currentIndex; + state._value = 'P' + ids.join('') + currentIndex; } return state._value; diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index 4d96b40e45..a78453bf67 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -55,12 +55,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); }); @@ -83,12 +83,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); }); @@ -121,13 +121,13 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); set(true); rerender(); expect(scratch.innerHTML).to.equal( - '
hh
' + '
hh
' ); }); @@ -350,7 +350,7 @@ describe('useId', () => { } render(, scratch); - expect(scratch.innerHTML).to.equal('

P481

P15671

'); + expect(scratch.innerHTML).to.equal('

P01

P011

'); }); it('should skip over HTML', () => { @@ -376,7 +376,7 @@ describe('useId', () => { } render(, scratch); - expect(ids).to.deep.equal(['P491', 'P501']); + expect(ids).to.deep.equal(['P11', 'P21']); }); it('should reset for each renderToString roots', () => { From 062d62a6927b4201e399f6893229588817ae9d59 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 20:42:56 +0200 Subject: [PATCH 5/8] Rename _ids -> _mask --- hooks/src/index.js | 3 ++- hooks/src/internal.d.ts | 4 ++++ mangle.json | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 931f9d7ad7..dc0a1d14ce 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -372,6 +372,7 @@ export function useId() { if (!state._value) { // Traverse the tree upwards and count the components until we reach // the root node. + /** @type {import('./internal.d').VNode} */ let root = currentComponent._vnode; let parent = root._parent; let i = 0; @@ -385,7 +386,7 @@ export function useId() { } // Attach the id usage array to the root node and resize it to fit - let ids = root._ids || (root._ids = [0]); + let ids = root._mask || (root._mask = [0]); while (ids.length < i) { ids.push(0); } diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index a555e4ec49..cda51af7d6 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -38,6 +38,10 @@ export interface Component extends PreactComponent { __hooks?: ComponentHooks; } +export interface VNode extends PreactVNode { + _mask?: number[]; +} + export type HookState = | EffectHookState | MemoHookState diff --git a/mangle.json b/mangle.json index e51b759649..22b96e72bf 100644 --- a/mangle.json +++ b/mangle.json @@ -39,6 +39,7 @@ "$_depth": "__b", "$_nextDom": "__d", "$_dirty": "__d", + "$_mask": "__m", "$_detachOnNextRender": "__b", "$_force": "__e", "$_nextState": "__s", From a15edaf36645d54e24d0908df1a4ebe037cb2512 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 22:07:30 +0200 Subject: [PATCH 6/8] Use counter per boundary for id generation --- hooks/src/index.js | 21 +++++---------------- hooks/src/internal.d.ts | 2 +- hooks/test/browser/useId.test.js | 19 +++++++++++-------- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index dc0a1d14ce..1772398b49 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -1,4 +1,4 @@ -import { Fragment, options } from 'preact'; +import { options } from 'preact'; /** @type {number} */ let currentIndex; @@ -374,26 +374,15 @@ export function useId() { // the root node. /** @type {import('./internal.d').VNode} */ let root = currentComponent._vnode; - let parent = root._parent; - let i = 0; - while (parent !== null) { - if (parent.type !== Fragment && typeof parent.type === 'function') { - i++; - } - - root = parent; - parent = parent._parent; + while (root !== null && root._parent !== null) { + root = root._parent; } // Attach the id usage array to the root node and resize it to fit - let ids = root._mask || (root._mask = [0]); - while (ids.length < i) { - ids.push(0); - } + let mask = root._mask || (root._mask = [0, 0]); // Increase the current component depth pointer - ids[i - 1]++; - state._value = 'P' + ids.join('') + currentIndex; + state._value = 'P' + mask[0] + mask[1]++ + currentIndex; } return state._value; diff --git a/hooks/src/internal.d.ts b/hooks/src/internal.d.ts index cda51af7d6..4d4be511f7 100644 --- a/hooks/src/internal.d.ts +++ b/hooks/src/internal.d.ts @@ -39,7 +39,7 @@ export interface Component extends PreactComponent { } export interface VNode extends PreactVNode { - _mask?: number[]; + _mask?: [number, number]; } export type HookState = diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index a78453bf67..c8aa3e1d0b 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -55,12 +55,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); }); @@ -83,12 +83,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); }); @@ -121,13 +121,13 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); set(true); rerender(); expect(scratch.innerHTML).to.equal( - '
hh
' + '
hh
' ); }); @@ -332,13 +332,16 @@ describe('useId', () => { return {children}; }; + const ids = []; function Foo() { const id = useId(); + ids.push(id); return

{id}

; } function App() { const id = useId(); + ids.push(id); return (

{id}

@@ -350,7 +353,7 @@ describe('useId', () => { } render(, scratch); - expect(scratch.innerHTML).to.equal('

P01

P011

'); + expect(ids[0]).not.to.equal(ids[1]); }); it('should skip over HTML', () => { @@ -376,7 +379,7 @@ describe('useId', () => { } render(, scratch); - expect(ids).to.deep.equal(['P11', 'P21']); + expect(ids[0]).not.to.equal(ids[1]); }); it('should reset for each renderToString roots', () => { From 253c39fd29d23bf3c9c45c9593c4adbf0512bde2 Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 22:17:12 +0200 Subject: [PATCH 7/8] Simplify useId --- hooks/src/index.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 1772398b49..70e0f265a3 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -370,18 +370,14 @@ export function useErrorBoundary(cb) { export function useId() { const state = getHookState(currentIndex++, 11); if (!state._value) { - // Traverse the tree upwards and count the components until we reach - // the root node. + // Grab either the root node or the nearest async boundary node. /** @type {import('./internal.d').VNode} */ let root = currentComponent._vnode; - while (root !== null && root._parent !== null) { + while (root !== null && !root._mask && root._parent !== null) { root = root._parent; } - // Attach the id usage array to the root node and resize it to fit let mask = root._mask || (root._mask = [0, 0]); - - // Increase the current component depth pointer state._value = 'P' + mask[0] + mask[1]++ + currentIndex; } From 979c4e68778a433ba23c66fd1a1697353eda21ea Mon Sep 17 00:00:00 2001 From: Marvin Hagemeister Date: Tue, 25 Oct 2022 23:08:55 +0200 Subject: [PATCH 8/8] Separate root id + id part to avoid ambuigity --- hooks/src/index.js | 2 +- hooks/test/browser/useId.test.js | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hooks/src/index.js b/hooks/src/index.js index 70e0f265a3..d05947eaaf 100644 --- a/hooks/src/index.js +++ b/hooks/src/index.js @@ -378,7 +378,7 @@ export function useId() { } let mask = root._mask || (root._mask = [0, 0]); - state._value = 'P' + mask[0] + mask[1]++ + currentIndex; + state._value = 'P' + mask[0] + '-' + mask[1]++; } return state._value; diff --git a/hooks/test/browser/useId.test.js b/hooks/test/browser/useId.test.js index c8aa3e1d0b..e7fc7f947b 100644 --- a/hooks/test/browser/useId.test.js +++ b/hooks/test/browser/useId.test.js @@ -55,12 +55,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); }); @@ -83,12 +83,12 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); render(, scratch); expect(scratch.innerHTML).to.equal( - '
hhh
' + '
hhh
' ); }); @@ -121,13 +121,13 @@ describe('useId', () => { render(, scratch); expect(scratch.innerHTML).to.equal( - '
h
' + '
h
' ); set(true); rerender(); expect(scratch.innerHTML).to.equal( - '
hh
' + '
hh
' ); });