Components: Simplify ScrollLock, adding types #29634
Conversation
if ( htmlDocument === undefined ) { | ||
// If in SSR context, for example, there will be no HTML document, so just return a component that does nothing | ||
return () => null; | ||
} | ||
|
sarayourfriend
Mar 8, 2021
Author
Contributor
I added this because I thought it made more sense to just "nope out" if htmlDocument
isn't present, but it doesn't resolve the null errors in the setLocked
function below. Why is that? @sirreal do you happen to know?
I added this because I thought it made more sense to just "nope out" if htmlDocument
isn't present, but it doesn't resolve the null errors in the setLocked
function below. Why is that? @sirreal do you happen to know?
jsnajdr
Mar 9, 2021
Contributor
Function declarations are hoisted up to the beginning of scope. So this condition doesn't skip them -- they effectively are before it, at the beginning of the function.
In this particular case, the htmlDocument
checks are not necessary. All the document-accessing code is ever run only in componentDidMount
/componentWillUnmount
lifecycles methods and these are guaranteed to never be called by the server renderer.
It would be nice if we could tell TypeScript somehow that in this case the htmlDocument
is always valid. With some lock( htmlDocument as HTMLDocument )
syntax maybe.
Function declarations are hoisted up to the beginning of scope. So this condition doesn't skip them -- they effectively are before it, at the beginning of the function.
In this particular case, the htmlDocument
checks are not necessary. All the document-accessing code is ever run only in componentDidMount
/componentWillUnmount
lifecycles methods and these are guaranteed to never be called by the server renderer.
It would be nice if we could tell TypeScript somehow that in this case the htmlDocument
is always valid. With some lock( htmlDocument as HTMLDocument )
syntax maybe.
jsnajdr
Mar 9, 2021
Contributor
Generally, I believe the way how this component is written is unnecessarily complicated. createScrollLockComponent
is not a part of public API (is not reexported by the index.js
module) and can be removed. Something like this should work perfectly well:
function setLocked( locked ) {
// naked reference to document. it's safe
document.body.scrollTop = ...
}
export default function ScrollLock() {
useEffect( () => {
setLocked( true );
return () => setLocked( false );
}, [] );
return null;
}
That's all. References to document
are safe even on a Node.js server.
Generally, I believe the way how this component is written is unnecessarily complicated. createScrollLockComponent
is not a part of public API (is not reexported by the index.js
module) and can be removed. Something like this should work perfectly well:
function setLocked( locked ) {
// naked reference to document. it's safe
document.body.scrollTop = ...
}
export default function ScrollLock() {
useEffect( () => {
setLocked( true );
return () => setLocked( false );
}, [] );
return null;
}
That's all. References to document
are safe even on a Node.js server.
sarayourfriend
Mar 9, 2021
Author
Contributor
I don't think it's that simple @jsnajdr. The component also tracks the number of scroll lock requests so that multiple scroll locks in the same page continue to work as expected.
I don't think it's that simple @jsnajdr. The component also tracks the number of scroll lock requests so that multiple scroll locks in the same page continue to work as expected.
jsnajdr
Mar 9, 2021
Contributor
Yes, there is reference counting with a module-global lockCounter
variable. setLocked
inc/decrements it and does anything only when it increased from 0 to 1 or decreases from 1 to 0. That shouldn't complicate the outline I shared very much.
Yes, there is reference counting with a module-global lockCounter
variable. setLocked
inc/decrements it and does anything only when it increased from 0 to 1 or decreases from 1 to 0. That shouldn't complicate the outline I shared very much.
Size Change: -220 B (0%) Total Size: 1.4 MB
|
Filename | Size | Change |
---|---|---|
build/a11y/index.js |
1.14 kB | 0 B |
build/blob/index.js |
664 B | 0 B |
build/block-directory/index.js |
8.63 kB | 0 B |
build/block-directory/style-rtl.css |
1 kB | 0 B |
build/block-directory/style.css |
1.01 kB | 0 B |
build/block-editor/style-rtl.css |
12.1 kB | 0 B |
build/block-editor/style.css |
12.1 kB | 0 B |
build/block-library/blocks/archives/editor-rtl.css |
61 B | 0 B |
build/block-library/blocks/archives/editor.css |
60 B | 0 B |
build/block-library/blocks/audio/editor-rtl.css |
58 B | 0 B |
build/block-library/blocks/audio/editor.css |
58 B | 0 B |
build/block-library/blocks/audio/style-rtl.css |
112 B | 0 B |
build/block-library/blocks/audio/style.css |
112 B | 0 B |
build/block-library/blocks/block/editor-rtl.css |
161 B | 0 B |
build/block-library/blocks/block/editor.css |
161 B | 0 B |
build/block-library/blocks/button/editor-rtl.css |
475 B | 0 B |
build/block-library/blocks/button/editor.css |
474 B | 0 B |
build/block-library/blocks/button/style-rtl.css |
479 B | 0 B |
build/block-library/blocks/button/style.css |
479 B | 0 B |
build/block-library/blocks/buttons/editor-rtl.css |
315 B | 0 B |
build/block-library/blocks/buttons/editor.css |
315 B | 0 B |
build/block-library/blocks/buttons/style-rtl.css |
364 B | 0 B |
build/block-library/blocks/buttons/style.css |
363 B | 0 B |
build/block-library/blocks/calendar/style-rtl.css |
208 B | 0 B |
build/block-library/blocks/calendar/style.css |
208 B | 0 B |
build/block-library/blocks/categories/editor-rtl.css |
84 B | 0 B |
build/block-library/blocks/categories/editor.css |
83 B | 0 B |
build/block-library/blocks/categories/style-rtl.css |
79 B | 0 B |
build/block-library/blocks/categories/style.css |
79 B | 0 B |
build/block-library/blocks/code/style-rtl.css |
90 B | 0 B |
build/block-library/blocks/code/style.css |
90 B | 0 B |
build/block-library/blocks/columns/editor-rtl.css |
190 B | 0 B |
build/block-library/blocks/columns/editor.css |
190 B | 0 B |
build/block-library/blocks/columns/style-rtl.css |
421 B | 0 B |
build/block-library/blocks/columns/style.css |
421 B | 0 B |
build/block-library/blocks/cover/editor-rtl.css |
390 B | 0 B |
build/block-library/blocks/cover/editor.css |
389 B | 0 B |
build/block-library/blocks/cover/style-rtl.css |
1.24 kB | 0 B |
build/block-library/blocks/cover/style.css |
1.24 kB | 0 B |
build/block-library/blocks/embed/editor-rtl.css |
486 B | 0 B |
build/block-library/blocks/embed/editor.css |
486 B | 0 B |
build/block-library/blocks/embed/style-rtl.css |
401 B | 0 B |
build/block-library/blocks/embed/style.css |
400 B | 0 B |
build/block-library/blocks/file/editor-rtl.css |
199 B | 0 B |
build/block-library/blocks/file/editor.css |
198 B | 0 B |
build/block-library/blocks/file/style-rtl.css |
248 B | 0 B |
build/block-library/blocks/file/style.css |
248 B | 0 B |
build/block-library/blocks/freeform/editor-rtl.css |
2.45 kB | 0 B |
build/block-library/blocks/freeform/editor.css |
2.45 kB | 0 B |
build/block-library/blocks/gallery/editor-rtl.css |
689 B | 0 B |
build/block-library/blocks/gallery/editor.css |
690 B | 0 B |
build/block-library/blocks/gallery/style-rtl.css |
1.08 kB | 0 B |
build/block-library/blocks/gallery/style.css |
1.07 kB | 0 B |
build/block-library/blocks/group/editor-rtl.css |
318 B | 0 B |
build/block-library/blocks/group/editor.css |
317 B | 0 B |
build/block-library/blocks/group/style-rtl.css |
57 B | 0 B |
build/block-library/blocks/group/style.css |
57 B | 0 B |
build/block-library/blocks/heading/editor-rtl.css |
129 B | 0 B |
build/block-library/blocks/heading/editor.css |
129 B | 0 B |
build/block-library/blocks/heading/style-rtl.css |
76 B | 0 B |
build/block-library/blocks/heading/style.css |
76 B | 0 B |
build/block-library/blocks/html/editor-rtl.css |
281 B | 0 B |
build/block-library/blocks/html/editor.css |
281 B | 0 B |
build/block-library/blocks/image/editor-rtl.css |
717 B | 0 B |
build/block-library/blocks/image/editor.css |
716 B | 0 B |
build/block-library/blocks/image/style-rtl.css |
476 B | 0 B |
build/block-library/blocks/image/style.css |
478 B | 0 B |
build/block-library/blocks/latest-comments/editor-rtl.css |
159 B | 0 B |
build/block-library/blocks/latest-comments/editor.css |
158 B | 0 B |
build/block-library/blocks/latest-comments/style-rtl.css |
269 B | 0 B |
build/block-library/blocks/latest-comments/style.css |
269 B | 0 B |
build/block-library/blocks/latest-posts/editor-rtl.css |
137 B | 0 B |
build/block-library/blocks/latest-posts/editor.css |
137 B | 0 B |
build/block-library/blocks/latest-posts/style-rtl.css |
523 B | 0 B |
build/block-library/blocks/latest-posts/style.css |
522 B | 0 B |
build/block-library/blocks/list/editor-rtl.css |
65 B | 0 B |
build/block-library/blocks/list/editor.css |
65 B | 0 B |
build/block-library/blocks/list/style-rtl.css |
63 B | 0 B |
build/block-library/blocks/list/style.css |
63 B | 0 B |
build/block-library/blocks/media-text/editor-rtl.css |
191 B | 0 B |
build/block-library/blocks/media-text/editor.css |
191 B | 0 B |
build/block-library/blocks/media-text/style-rtl.css |
535 B | 0 B |
build/block-library/blocks/media-text/style.css |
532 B | 0 B |
build/block-library/blocks/more/editor-rtl.css |
434 B | 0 B |
build/block-library/blocks/more/editor.css |
434 B | 0 B |
build/block-library/blocks/navigation-link/editor-rtl.css |
681 B | 0 B |
build/block-library/blocks/navigation-link/editor.css |
683 B | 0 B |
build/block-library/blocks/navigation-link/style-rtl.css |
694 B | 0 B |
build/block-library/blocks/navigation-link/style.css |
692 B | 0 B |
build/block-library/blocks/navigation/editor-rtl.css |
1.34 kB | 0 B |
build/block-library/blocks/navigation/editor.css |
1.34 kB | 0 B |
build/block-library/blocks/navigation/style-rtl.css |
213 B | 0 B |
build/block-library/blocks/navigation/style.css |
214 B | 0 B |
build/block-library/blocks/nextpage/editor-rtl.css |
395 B | 0 B |
build/block-library/blocks/nextpage/editor.css |
395 B | 0 B |
build/block-library/blocks/page-list/editor-rtl.css |
215 B | 0 B |
build/block-library/blocks/page-list/editor.css |
215 B | 0 B |
build/block-library/blocks/page-list/style-rtl.css |
527 B | 0 B |
build/block-library/blocks/page-list/style.css |
526 B | 0 B |
build/block-library/blocks/paragraph/editor-rtl.css |
157 B | 0 B |
build/block-library/blocks/paragraph/editor.css |
157 B | 0 B |
build/block-library/blocks/paragraph/style-rtl.css |
288 B | 0 B |
build/block-library/blocks/paragraph/style.css |
289 B | 0 B |
build/block-library/blocks/post-author/editor-rtl.css |
209 B | 0 B |
build/block-library/blocks/post-author/editor.css |
209 B | 0 B |
build/block-library/blocks/post-author/style-rtl.css |
183 B | 0 B |
build/block-library/blocks/post-author/style.css |
184 B | 0 B |
build/block-library/blocks/post-comments-form/style-rtl.css |
250 B | 0 B |
build/block-library/blocks/post-comments-form/style.css |
250 B | 0 B |
build/block-library/blocks/post-content/editor-rtl.css |
139 B | 0 B |
build/block-library/blocks/post-content/editor.css |
139 B | 0 B |
build/block-library/blocks/post-excerpt/editor-rtl.css |
73 B | 0 B |
build/block-library/blocks/post-excerpt/editor.css |
73 B | 0 B |
build/block-library/blocks/post-featured-image/editor-rtl.css |
338 B | 0 B |
build/block-library/blocks/post-featured-image/editor.css |
338 B | 0 B |
build/block-library/blocks/post-featured-image/style-rtl.css |
100 B | 0 B |
build/block-library/blocks/post-featured-image/style.css |
100 B | 0 B |
build/block-library/blocks/preformatted/style-rtl.css |
63 B | 0 B |
build/block-library/blocks/preformatted/style.css |
63 B | 0 B |
build/block-library/blocks/pullquote/editor-rtl.css |
183 B | 0 B |
build/block-library/blocks/pullquote/editor.css |
183 B | 0 B |
build/block-library/blocks/pullquote/style-rtl.css |
318 B | 0 B |
build/block-library/blocks/pullquote/style.css |
318 B | 0 B |
build/block-library/blocks/query-loop/editor-rtl.css |
90 B | 0 B |
build/block-library/blocks/query-loop/editor.css |
89 B | 0 B |
build/block-library/blocks/query-loop/style-rtl.css |
315 B | 0 B |
build/block-library/blocks/query-loop/style.css |
317 B | 0 B |
build/block-library/blocks/query-pagination-numbers/editor-rtl.css |
122 B | 0 B |
build/block-library/blocks/query-pagination-numbers/editor.css |
121 B | 0 B |
build/block-library/blocks/query-pagination/editor-rtl.css |
270 B | 0 B |
build/block-library/blocks/query-pagination/editor.css |
262 B | 0 B |
build/block-library/blocks/query-pagination/style-rtl.css |
168 B | 0 B |
build/block-library/blocks/query-pagination/style.css |
168 B | 0 B |
build/block-library/blocks/query/editor-rtl.css |
814 B | 0 B |
build/block-library/blocks/query/editor.css |
812 B | 0 B |
build/block-library/blocks/quote/editor-rtl.css |
61 B | 0 B |
build/block-library/blocks/quote/editor.css |
61 B | 0 B |
build/block-library/blocks/quote/style-rtl.css |
169 B | 0 B |
build/block-library/blocks/quote/style.css |
169 B | 0 B |
build/block-library/blocks/rss/editor-rtl.css |
201 B | 0 B |
build/block-library/blocks/rss/editor.css |
202 B | 0 B |
build/block-library/blocks/rss/style-rtl.css |
290 B | 0 B |
build/block-library/blocks/rss/style.css |
290 B | 0 B |
build/block-library/blocks/search/editor-rtl.css |
165 B | 0 B |
build/block-library/blocks/search/editor.css |
165 B | 0 B |
build/block-library/blocks/search/style-rtl.css |
342 B | 0 B |
build/block-library/blocks/search/style.css |
344 B | 0 B |
build/block-library/blocks/separator/editor-rtl.css |
99 B | 0 B |
build/block-library/blocks/separator/editor.css |
99 B | 0 B |
build/block-library/blocks/separator/style-rtl.css |
236 B | 0 B |
build/block-library/blocks/separator/style.css |
236 B | 0 B |
build/block-library/blocks/shortcode/editor-rtl.css |
504 B | 0 B |
build/block-library/blocks/shortcode/editor.css |
504 B | 0 B |
build/block-library/blocks/site-logo/editor-rtl.css |
201 B | 0 B |
build/block-library/blocks/site-logo/editor.css |
201 B | 0 B |
build/block-library/blocks/site-logo/style-rtl.css |
115 B | 0 B |
build/block-library/blocks/site-logo/style.css |
115 B | 0 B |
build/block-library/blocks/social-link/editor-rtl.css |
164 B | 0 B |
build/block-library/blocks/social-link/editor.css |
165 B | 0 B |
build/block-library/blocks/social-links/editor-rtl.css |
696 B | 0 B |
build/block-library/blocks/social-links/editor.css |
696 B | 0 B |
build/block-library/blocks/social-links/style-rtl.css |
1.32 kB | 0 B |
build/block-library/blocks/social-links/style.css |
1.32 kB | 0 B |
build/block-library/blocks/spacer/editor-rtl.css |
317 B | 0 B |
build/block-library/blocks/spacer/editor.css |
317 B | 0 B |
build/block-library/blocks/spacer/style-rtl.css |
48 B | 0 B |
build/block-library/blocks/spacer/style.css |
48 B | 0 B |
build/block-library/blocks/table/editor-rtl.css |
478 B | 0 B |
build/block-library/blocks/table/editor.css |
478 B | 0 B |
build/block-library/blocks/table/style-rtl.css |
402 B | 0 B |
build/block-library/blocks/table/style.css |
402 B | 0 B |
build/block-library/blocks/tag-cloud/editor-rtl.css |
118 B | 0 B |
build/block-library/blocks/tag-cloud/editor.css |
118 B | 0 B |
build/block-library/blocks/tag-cloud/style-rtl.css |
94 B | 0 B |
build/block-library/blocks/tag-cloud/style.css |
94 B | 0 B |
build/block-library/blocks/template-part/editor-rtl.css |
557 B | 0 B |
build/block-library/blocks/template-part/editor.css |
556 B | 0 B |
build/block-library/blocks/text-columns/editor-rtl.css |
95 B | 0 B |
build/block-library/blocks/text-columns/editor.css |
95 B | 0 B |
build/block-library/blocks/text-columns/style-rtl.css |
166 B | 0 B |
build/block-library/blocks/text-columns/style.css |
166 B | 0 B |
build/block-library/blocks/verse/editor-rtl.css |
62 B | 0 B |
build/block-library/blocks/verse/editor.css |
62 B | 0 B |
build/block-library/blocks/verse/style-rtl.css |
87 B | 0 B |
build/block-library/blocks/verse/style.css |
87 B | 0 B |
build/block-library/blocks/video/editor-rtl.css |
504 B | 0 B |
build/block-library/blocks/video/editor.css |
503 B | 0 B |
build/block-library/blocks/video/style-rtl.css |
187 B | 0 B |
build/block-library/blocks/video/style.css |
187 B | 0 B |
build/block-library/common-rtl.css |
1.1 kB | 0 B |
build/block-library/common.css |
1.1 kB | 0 B |
build/block-library/editor-rtl.css |
9.54 kB | 0 B |
build/block-library/editor.css |
9.53 kB | 0 B |
build/block-library/style-rtl.css |
8.83 kB | 0 B |
build/block-library/style.css |
8.84 kB | 0 B |
build/block-library/theme-rtl.css |
736 B | 0 B |
build/block-library/theme.css |
736 B | 0 B |
build/block-serialization-spec-parser/index.js |
3.06 kB | 0 B |
build/components/style-rtl.css |
15.6 kB | 0 B |
build/components/style.css |
15.6 kB | 0 B |
build/customize-widgets/style-rtl.css |
168 B | 0 B |
build/customize-widgets/style.css |
168 B | 0 B |
build/data-controls/index.js |
828 B | 0 B |
build/data/index.js |
8.88 kB | 0 B |
build/date/index.js |
31.8 kB | 0 B |
build/deprecated/index.js |
769 B | 0 B |
build/dom-ready/index.js |
577 B | 0 B |
build/dom/index.js |
4.94 kB | 0 B |
build/edit-navigation/index.js |
11.8 kB | 0 B |
build/edit-navigation/style-rtl.css |
1.28 kB | 0 B |
build/edit-navigation/style.css |
1.28 kB | 0 B |
build/edit-post/style-rtl.css |
6.81 kB | 0 B |
build/edit-post/style.css |
6.8 kB | 0 B |
build/edit-site/style-rtl.css |
4.47 kB | 0 B |
build/edit-site/style.css |
4.46 kB | 0 B |
build/edit-widgets/style-rtl.css |
3.2 kB | 0 B |
build/edit-widgets/style.css |
3.2 kB | 0 B |
build/editor/editor-styles-rtl.css |
347 B | 0 B |
build/editor/editor-styles.css |
347 B | 0 B |
build/editor/style-rtl.css |
3.9 kB | 0 B |
build/editor/style.css |
3.9 kB | 0 B |
build/element/index.js |
4.61 kB | 0 B |
build/escape-html/index.js |
735 B | 0 B |
build/format-library/index.js |
6.75 kB | 0 B |
build/format-library/style-rtl.css |
637 B | 0 B |
build/format-library/style.css |
639 B | 0 B |
build/hooks/index.js |
2.28 kB | 0 B |
build/html-entities/index.js |
622 B | 0 B |
build/i18n/index.js |
4.01 kB | 0 B |
build/is-shallow-equal/index.js |
699 B | 0 B |
build/list-reusable-blocks/index.js |
3.15 kB | 0 B |
build/list-reusable-blocks/style-rtl.css |
629 B | 0 B |
build/list-reusable-blocks/style.css |
628 B | 0 B |
build/nux/style-rtl.css |
731 B | 0 B |
build/nux/style.css |
727 B | 0 B |
build/primitives/index.js |
1.42 kB | 0 B |
build/priority-queue/index.js |
791 B | 0 B |
build/redux-routine/index.js |
2.83 kB | 0 B |
build/reusable-blocks/index.js |
3.78 kB | 0 B |
build/reusable-blocks/style-rtl.css |
225 B | 0 B |
build/reusable-blocks/style.css |
225 B | 0 B |
build/shortcode/index.js |
1.7 kB | 0 B |
build/token-list/index.js |
1.27 kB | 0 B |
build/warning/index.js |
1.14 kB | 0 B |
build/wordcount/index.js |
1.22 kB | 0 B |
Aside from the type fixes, I'm not 100% sure those secondary Is there any good way to reproduce the errors against which we're trying to guard here? In other words, how did you know "what and where to fix"? |
The TypeScript compiler will complain about cases where variables can be undefined, so I covered those cases. If you wish to reproduce them, simply remove the null checks. I can do the refactor that @jsnajdr suggested, it may simplify things when we go back to add types. |
if ( typeof document === undefined ) { | ||
// if in SSR context, don't do anything | ||
return; | ||
} |
sarayourfriend
Mar 9, 2021
Author
Contributor
Do we need this check? I can never remember what runs in SRR, but if componentDidMount
runs then we'll need this right, otherwise setLocked
will run and document
will be undefined.
Do we need this check? I can never remember what runs in SRR, but if componentDidMount
runs then we'll need this right, otherwise setLocked
will run and document
will be undefined.
jsnajdr
Mar 9, 2021
Contributor
SSR runs only the constructor
and the rare componentWillMount
, nothing else. didMount
is already an effect (equivalent mostly to useEffect
) and SSR doesn't do effects.
SSR runs only the constructor
and the rare componentWillMount
, nothing else. didMount
is already an effect (equivalent mostly to useEffect
) and SSR doesn't do effects.
sarayourfriend
Mar 9, 2021
Author
Contributor
Gotcha, thanks for confirming. I'll remove the extra check.
Gotcha, thanks for confirming. I'll remove the extra check.
export default function ScrollLock( { className = 'lockscroll' } ) { | ||
const setLocked = useSetLocked( className ); | ||
|
||
useEffect( () => { | ||
if ( lockCounter === 0 ) { |
jsnajdr
Mar 10, 2021
Contributor
Can be shortened to:
if ( lockCounter++ === 0 ) setLocked( true );
return () => { if ( --lockCounter === 0 ) setLocked( false ); }
Can be shortened to:
if ( lockCounter++ === 0 ) setLocked( true );
return () => { if ( --lockCounter === 0 ) setLocked( false ); }
let previousScrollTop = 0; | ||
function useSetLocked( className ) { | ||
/** @type {import('react').MutableRefObject<number>} */ | ||
const previousScrollTop = useRef( 0 ); |
jsnajdr
Mar 10, 2021
Contributor
The ref means that each instance of <ScrollLock />
will have its own previousScrollTop
instance.
If the scroll is locked by one instance and then unlocked by another, it will fail to restore previousScrollTop
.
The ref means that each instance of <ScrollLock />
will have its own previousScrollTop
instance.
If the scroll is locked by one instance and then unlocked by another, it will fail to restore previousScrollTop
.
} | ||
--lockCounter; | ||
}; | ||
}, [] ); |
jsnajdr
Mar 10, 2021
Contributor
The react-hooks/exhaustive-deps
rule is going to complain about missing setLocked
here.
The react-hooks/exhaustive-deps
rule is going to complain about missing setLocked
here.
sarayourfriend commentedMar 8, 2021
•
edited
Description
Refactors and simplifies
ScrollLock
to remove the unnecessarycreateScrollLockComponent
function and also add types.How has this been tested?
Type check passes, scroll lock continues to work (you can test this in storybook).
Types of changes
Non-breaking changes.
Checklist: