Skip to content

Components: Fix React Compiler error for 'useScrollRectIntoView'#66498

Merged
Mamaduka merged 3 commits into
trunkfrom
fix/tablist-compiler-error
Nov 4, 2024
Merged

Components: Fix React Compiler error for 'useScrollRectIntoView'#66498
Mamaduka merged 3 commits into
trunkfrom
fix/tablist-compiler-error

Conversation

@Mamaduka

Copy link
Copy Markdown
Member

What?

Part of #61788.
Similar to #66492.

PR fixes React Compiler errors for the useScrollRectIntoView hook using Element.scroll instead of the shorthand method.

Testing Instructions

Smoke test Tabs components in Storybook and editors.

@Mamaduka Mamaduka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 26, 2024
@Mamaduka Mamaduka self-assigned this Oct 26, 2024
@Mamaduka Mamaduka requested a review from ajitbohra as a code owner October 26, 2024 11:46
@Mamaduka Mamaduka requested review from ciampo and tyxla October 26, 2024 11:46
@github-actions

github-actions Bot commented Oct 26, 2024

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo

ciampo commented Oct 28, 2024

Copy link
Copy Markdown
Contributor

@Mamaduka unit tests are failing because jsdom doesn't implement scroll-related methods (see jsdom/jsdom#1422 and jsdom/jsdom#2751) — I suspect this was the original cause that pushed Dani to use scrollLeft instead of scroll().

We could either look into adding global mocks for the whole Gutengerg project, or potentially add extra checks (or optional chaining) in the source code and add a comment explaining the reason for it.

@tyxla

tyxla commented Oct 28, 2024

Copy link
Copy Markdown
Member

IMHO, those mutations should be allowed by the React Compiler, since all those Element APIs are perfectly valid, and the non-readonly properties should be writable, and not considered violations. Perhaps we should consider raising an issue in the React repo?

@Mamaduka

Copy link
Copy Markdown
Member Author

IMHO, those mutations should be allowed by the React Compiler.

That would be ideal, but the same issue remains with the refs (#29196). I've got a feeling it's a complex problem to resolve, and using Element methods (and similar) might be the only current solution.

@Mamaduka Mamaduka force-pushed the fix/tablist-compiler-error branch from 4ceb079 to 925b5f5 Compare November 1, 2024 04:37
@Mamaduka

Mamaduka commented Nov 1, 2024

Copy link
Copy Markdown
Member Author

Updated in 925b5f5.

@github-actions

github-actions Bot commented Nov 1, 2024

Copy link
Copy Markdown

Flaky tests detected in 925b5f5.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11624389497
📝 Reported issues:

@tyxla tyxla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Mamaduka.

Just a minor optional suggestion

parent.scroll?.( { left: parentScroll - leftOverflow } );
} else if ( rightOverflow > 0 ) {
parent.scrollLeft = parentScroll + rightOverflow;
parent.scroll?.( { left: parentScroll + rightOverflow } );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely the comment might make sense to be here too.

I wonder if we can isolate the logic to a single parent?.scroll() call. Then, it will be just a single comment.

scrollLeft = parentScroll + rightOverflow;
}

if ( scrollLeft !== null ) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a precaution.

@tyxla tyxla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Mamaduka!

@Mamaduka Mamaduka merged commit b992649 into trunk Nov 4, 2024
@Mamaduka Mamaduka deleted the fix/tablist-compiler-error branch November 4, 2024 11:56
@github-actions github-actions Bot added this to the Gutenberg 19.7 milestone Nov 4, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…dPress#66498)

* Components: Fix React Compiler error for 'useScrollRectIntoView'
* Use optional chaining and add inline comment
* Use single scroll call

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants