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

[Core] Suggestion to fix eslint error with CodeCopy.tsx with immutable object #44348

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wilhelmlofsten
Copy link
Contributor

@wilhelmlofsten wilhelmlofsten commented Nov 8, 2024

Suggestion for fix #42564 with CodeCopy.tsx.

So suggestion with using React Callback with passing the argument instead of directly and due to rootNode.current = node still being immutable (eslint error), can disable it with react compiler - saw that it was done previously in the code. So open for suggestions for this solution/ feedback to change the code! 😃

@mui-bot
Copy link

mui-bot commented Nov 8, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b254db9

@zannager zannager added the core Infrastructure work going on behind the scenes label Nov 8, 2024
@zannager zannager requested a review from Janpot November 8, 2024 11:12
@Janpot
Copy link
Member

Janpot commented Nov 8, 2024

I'm not sure what we gain from this refactor, the code still contains an ignore directive, which means the whole file still doesn't get transformed.

I believe the canonical solution to this problem would be to expose the setNode in a context instead of the ref. But I could be wrong, I have just as little mileage with React 19 as the next guy 🙂

@wilhelmlofsten
Copy link
Contributor Author

I'm not sure what we gain from this refactor, the code still contains an ignore directive, which means the whole file still doesn't get transformed.

I believe the canonical solution to this problem would be to expose the setNode in a context instead of the ref. But I could be wrong, I have just as little mileage with React 19 as the next guy 🙂

Yea noticed CI testlint failing now when i turned the eslintrc check back to false again with "// eslint-disable-next-line react-compiler/react-compiler" with now giving errors ..

I would argue that im still new to open source development and MUI project, so thankful for the fast feedback! Will look into exposing the setNode into the context instead with that seeming to be a more thoughtful approach! 😄

@Janpot
Copy link
Member

Janpot commented Nov 8, 2024

yes, but even that, isn't this just a false positive in the eslint plugin? I assume it's only complaining because it can't infer that in fact rootNode is a mutable ref.

@wilhelmlofsten
Copy link
Contributor Author

yes, but even that, isn't this just a false positive in the eslint plugin? I assume it's only complaining because it can't infer that in fact rootNode is a mutable ref.

Yeah, that makes sense - it does seem like a false positive, especially since setting eslint.rc to false resolves it. And yes, rootNode is indeed mutable. Should we consider adjusting the ESLint configuration if the inline comment isn’t working as intended, or how should we proceed forward?

…Callback with passing the argument instead of directly and due to rootNode.current = node still being immutable (eslint error), can disable it with react compiler - saw that it was done previously in the code.
@wilhelmlofsten
Copy link
Contributor Author

wilhelmlofsten commented Nov 11, 2024

Suggested push from feedback from @Janpot with:

  • changed setRootNode to setNode (my previous pr commit)
  • exposed the setNode inside the CodeBlockContext and passing arguments to setNode directly
  • added useMemo to prevent rerendering inside the codeCopy provider with contextValue

This removed the eslint error and tested it with compiling and used codeCopy and seems to work 😃

So open for feedback/suggestions/ Reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] Fix eslint-plugin-react-compiler issues
4 participants