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

feat(eslint-config): new rulе no-modifying-ref-on-render #37

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

SeanSilke
Copy link
Contributor

@SeanSilke SeanSilke commented Mar 13, 2024

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @salutejs/[email protected]
# or 
yarn add @salutejs/[email protected]

@SeanSilke SeanSilke requested a review from KateKate March 13, 2024 19:11
function reportError(context, node) {
context.report({
node,
message: 'Do not modifying ref on render',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Давай как в доке - Do not write ref.current during rendering

https://react.dev/reference/react/useRef#useref:~:text=Next%20Example-,Pitfall,-Do%20not%20write

return <div>text</div>;
}`,
},
],
Copy link
Collaborator

@KateKate KateKate Mar 14, 2024

Choose a reason for hiding this comment

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

copilot подсказал

{
            code: `
            const MyComponent = (props) => {
                const ref = useRef(props);
                return <div>text</div>;
            }`,
        },
        
        {
            code: `
            const MyComponent = (props) => {
                const ref = useRef(props);
                useEffect(() => {
                    // Do something
                }, []);
                return <div>text</div>;
            }`,
        },

Copy link
Collaborator

Choose a reason for hiding this comment

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

{
            code: `
            const MyComponent = (props) => {
                const ref = useRef(props);
                useEffect(() => {
                    ref.current = props;
                }, [props]);
                return <div>text</div>;
            }`,
        },

}`,
errors: [errorMsg()],
},
],
Copy link
Collaborator

@KateKate KateKate Mar 14, 2024

Choose a reason for hiding this comment

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

а это вроде уже сложно?

        {
            code: `
            const MyComponent = (props) => {
                const ref = useRef(props);
                useEffect(() => {
                    ref.current = props;
                }, []);
                return <div>text</div>;
            }`,
        },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ту не понял. Код валидный.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ref-ку надо обычно обновлять на каждый рендер. А в данном случае обновляется только на первый рендер - это не имеет смысла, так как на первый рендер мы и так кладем значение через useRef. Но давай забьем, это хитрый кейс - вряд ли кто-то так пишет. Это мои фантазии )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ага ясно, пустые зависимости в useEffect. Да действительно редкий случай.

Copy link
Collaborator

@KateKate KateKate left a comment

Choose a reason for hiding this comment

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

.

@SeanSilke SeanSilke merged commit 6628d7e into master Mar 14, 2024
5 checks passed
@SeanSilke SeanSilke deleted the no_ref_on_render branch March 14, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants