Skip to content
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

[docs-infra] Fix React Compiler ESLint issues in website components #42566

Open
wants to merge 13 commits into
base: next
Choose a base branch
from

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Jun 7, 2024

Part of #42564

Fixes or silences errors reported by eslint-plugin-react-compiler in the website components.

The silenced occurrences are prepended with a comment because we use the --report-unused-disable-directives and we can't silence rules that are not configured, like the ones coming from eslint-plugin-react-compiler, which is disabled by default as of today.

// TODO: uncomment once we enable eslint-plugin-react-compiler // eslint-disable-next-line react-compiler/react-compiler -- useEnhancedEffect uses useEffect under the hood

Apart from that, add a missing React key.

How to test

Make sure the parts of the docs that are affected by the changes work as expected. Half of the changes are just silencing the corresponding ESLint rule.

@aarongarciah aarongarciah added the scope: docs-infra Specific to the docs-infra product label Jun 7, 2024
@mui-bot
Copy link

mui-bot commented Jun 7, 2024

Netlify deploy preview

https://deploy-preview-42566--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against e861ac6

@@ -101,7 +101,10 @@ export const useResizeHandle = (
if (target.current && dragging && clientX) {
const objectRect = target.current.getBoundingClientRect();
const newWidth = clientX - objectRect.left + dragOffset;
target.current.style.width = `clamp(${minWidth}, ${Math.floor(newWidth)}px, ${maxWidth})`;
target.current.style.setProperty(
Copy link
Member Author

Choose a reason for hiding this comment

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

Using style.setProperty so there's no "mutation".

Copy link
Member

Choose a reason for hiding this comment

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

This feels strange: facebook/react#29832.

@aarongarciah aarongarciah marked this pull request as ready for review June 7, 2024 15:48
Comment on lines +81 to +84
const rowGroupingCounterRef = React.useRef(0);
const isGroupExpandedByDefault = React.useCallback(() => {
// eslint-disable-next-line react-hooks/exhaustive-deps
rowGroupingCounter += 1;
return rowGroupingCounter === 3;
rowGroupingCounterRef.current += 1;
return rowGroupingCounterRef.current === 3;
Copy link
Member Author

@aarongarciah aarongarciah Jun 7, 2024

Choose a reason for hiding this comment

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

This is probably the only potentially sensitive change, but looks like the behavior is the same.

@aarongarciah
Copy link
Member Author

aarongarciah commented Jun 7, 2024

Looking into the reported ESLint errors:

Definition for rule 'react-compiler/react-compiler' was not found react-compiler/react-compiler

We use the --report-unused-disable-directives ESLint flag so we can't disable rules that are not configured and we only enable the eslint-plugin-react-compiler rules manually on local development for now.

@aarongarciah aarongarciah added the on hold There is a blocker, we need to wait label Jun 7, 2024
@aarongarciah
Copy link
Member Author

I've pushed a change commenting the ESLint disable comments for the react-compiler rules e.g.

- // eslint-disable-next-line react-compiler/react-compiler -- useEnhancedEffect uses useEffect under the hood
+ // TODO: uncomment once we enable eslint-plugin-react-compiler // eslint-disable-next-line react-compiler/react-compiler -- useEnhancedEffect uses useEffect under the hood

We can't have disabled rules that are not configured because we use the --report-unused-disable-directives ESLint flag.

@aarongarciah aarongarciah added website Pages that are not documentation-related, marketing-focused. and removed on hold There is a blocker, we need to wait labels Jun 10, 2024
docs/src/components/banner/AppFrameBanner.tsx Outdated Show resolved Hide resolved
@@ -101,7 +101,10 @@ export const useResizeHandle = (
if (target.current && dragging && clientX) {
const objectRect = target.current.getBoundingClientRect();
const newWidth = clientX - objectRect.left + dragOffset;
target.current.style.width = `clamp(${minWidth}, ${Math.floor(newWidth)}px, ${maxWidth})`;
target.current.style.setProperty(
Copy link
Member

Choose a reason for hiding this comment

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

This feels strange: facebook/react#29832.

@aarongarciah aarongarciah force-pushed the website-fix-eslint-react-compiler-issues branch from 7d56792 to e861ac6 Compare June 10, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: docs-infra Specific to the docs-infra product website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants