Fix errors and warnings detected by eslint. - #1389
Conversation
# Conflicts: # znai-reactjs/package.json # znai-reactjs/tsconfig.node.json
antoinell
changed the title
| return highlightAsList.length; | ||
| } | ||
|
|
||
| export const presentationSnippetHandler = { |
There was a problem hiding this comment.
let's move handlers like this into separate files. Not sure why it was put into utils. I think originally components and their optional presentation handlers were in the same place. What motivated to move it out?
There was a problem hiding this comment.
The eslint rule react-refresh/only-export-components motivated to move the handler out.
Would you prefer to have the handler in yet another separate file called for instance snippetHandler ?
There was a problem hiding this comment.
I am not sure why we need to follow that rule? What does it gives us? a doc element file contains both component to render the element for regular docs and a handler to render it for presentation mode. Two logically belong together. Unless the lint rule saves us from some hidden bugs I vote to remove the lint rule.
There was a problem hiding this comment.
The react-refresh/only-export-components ESLint rule ensures that Fast Refresh (React's hot module replacement) works correctly in your development environment.
What It Does
This rule enforces that files exporting React components should only export components (and optionally constants). It prevents you from mixing component exports with other exports that could break Fast Refresh.
Why It Exists
Fast Refresh works by re-rendering components when their code changes, without losing component state. However, it can only work reliably when:
The module exports are predictable
Side effects are minimized
Non-component logic doesn't interfere with the refresh boundary
There was a problem hiding this comment.
I would personally vote to keep this rule and apply it everywhere.
There was a problem hiding this comment.
does this fast refresh affect you? I feel like I haven't encountered it. To me it sounds like "in order to make a dev tool work correctly, we will force functions and components that logically belong together to be in separate files instead of fixing the tool".
There was a problem hiding this comment.
The fast refresh does not affect me. I have the same feeling like you that the refactoring forced by the eslint rule is unpleasant. I will deactivate the rule and undo the changes that I did because of this rule.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright 2021 TWO SIGMA OPEN SOURCE, LLC | |||
| * Copyright 2025 znai maintainers | |||
There was a problem hiding this comment.
this diff looks like we removed copyright. maybe I am look not at the latest diff
There was a problem hiding this comment.
Yes this looks like a mistake.
There was a problem hiding this comment.
In fact I stick by this edit, the TWO SIGMA copyright line was duplicated in this file.
| * limitations under the License. | ||
| */ | ||
|
|
||
| /* eslint-disable react-refresh/only-export-components */ |
There was a problem hiding this comment.
could you please tell me why do we need this?
There was a problem hiding this comment.
So there is a rule which disallows to export React components and other sort of functions from the same source file, but it is a lot of work to do the necessary change, so I did not do it for all files yet, I wanted to see whether this is a change you consider desirable or not.
|
We want to allow |
| const documentationTracker = { | ||
| onPageOpen(pageId) { | ||
| console.log("onPageOpen", pageId); | ||
| console.warn("onPageOpen", pageId); |
There was a problem hiding this comment.
Does this file count as a test or demo file ?
2 participants
In order to prevent regressions, the objective of this pull request is to add eslint to the build.
There are 0 warnings left in this version of the pull request.