feat(ui): Add ESLint with plugins for react/hooks, prettier, flow#25716
Closed
johallar wants to merge 8 commits intoprestodb:masterfrom
Closed
feat(ui): Add ESLint with plugins for react/hooks, prettier, flow#25716johallar wants to merge 8 commits intoprestodb:masterfrom
johallar wants to merge 8 commits intoprestodb:masterfrom
Conversation
johallar
changed the title
johallar
changed the title
johallar
force-pushed
the
eslint_with_flow
branch
2 times, most recently
from
2f48ab6 to
fe77f47
Compare
johallar
force-pushed
the
eslint_with_flow
branch
from
fe77f47 to
c2fb5e5
Compare
Contributor
Reviewer's GuideThis PR integrates ESLint with Prettier, React/React Hooks, and Flow plugins into the UI codebase by adding devDependencies, npm scripts, a new ESLint config file, and initial Flow annotations, without altering any runtime behavior. Class diagram for ESLint configuration structureclassDiagram
class ESLintConfig {
+ignores: string[]
+languageOptions: object
+plugins: object
+settings: object
+rules: object
+files: string[]
}
class PrettierPlugin {
+recommended: object
}
class ReactPlugin {
+recommended: object
+rules: object
}
class ReactHooksPlugin {
+recommended-latest: object
}
class FtFlowPlugin {
+recommended: object
}
class HermesParser {}
ESLintConfig --> PrettierPlugin
ESLintConfig --> ReactPlugin
ESLintConfig --> ReactHooksPlugin
ESLintConfig --> FtFlowPlugin
ESLintConfig --> HermesParser
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider adding the lint step to your CI pipeline now so that this new ESLint/prettier configuration is applied and validated automatically rather than as a separate follow-up.
- Renaming eslint.config.mjs to a more common config filename (like .eslintrc.cjs or .eslintrc.js) can help editors and tooling detect your ESLint settings out of the box.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding the lint step to your CI pipeline now so that this new ESLint/prettier configuration is applied and validated automatically rather than as a separate follow-up.
- Renaming eslint.config.mjs to a more common config filename (like .eslintrc.cjs or .eslintrc.js) can help editors and tooling detect your ESLint settings out of the box.
## Individual Comments
### Comment 1
<location> `presto-ui/src/components/QuerySplitsView.jsx:122-126` </location>
<code_context>
+ updateTimeline();
+ }
- useEffect(() => {
- if (data && show) {
- calculateItemsGroups();
- }
- }, [data, show]);
+ useEffect(() => {
+ if (data && show) {
</code_context>
<issue_to_address>
**suggestion:** Consider guarding against null containerRef in useEffect.
Add a check to ensure containerRef.current is not null before calling calculateItemsGroups to prevent timeline initialization errors.
</issue_to_address>
### Comment 2
<location> `presto-ui/src/d3utils.js:21-26` </location>
<code_context>
- return new dagreD3.graphlib.Graph({compound: true})
- .setGraph({rankdir: 'BT'})
- .setDefaultEdgeLabel(function () { return {}; });
+export function initializeGraph() {
+ return new dagreD3.graphlib.Graph({ compound: true })
+ .setGraph({ rankdir: "BT" })
+ .setDefaultEdgeLabel(function () {
+ return {};
+ });
}
</code_context>
<issue_to_address>
**nitpick:** Return type annotation for initializeGraph is missing.
Please add a Flow return type annotation to initializeGraph for better type safety.
</issue_to_address>
### Comment 3
<location> `presto-ui/src/components/QueryPlanView.jsx:119` </location>
<code_context>
const svg = widgets.current.svg;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {svg} = widgets.current;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 4
<location> `presto-ui/src/components/QuerySplitsView.jsx:24-122` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 5
<location> `presto-ui/src/components/SQLInput.jsx:103-107` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 6
<location> `presto-ui/src/components/SQLInput.jsx:212-226` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 7
<location> `presto-ui/src/components/SQLInput.jsx:233-242` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 8
<location> `presto-ui/src/components/Splits.jsx:31-143` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 9
<location> `presto-ui/src/components/Splits.jsx:145-161` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
122
to
126
| } | ||
|
|
||
| useEffect(() => { | ||
| if (data && show) { | ||
| calculateItemsGroups(); | ||
| } | ||
| }, [data, show]); | ||
| useEffect(() => { | ||
| if (data && show) { | ||
| calculateItemsGroups(); |
Contributor
There was a problem hiding this comment.
suggestion: Consider guarding against null containerRef in useEffect.
Add a check to ensure containerRef.current is not null before calling calculateItemsGroups to prevent timeline initialization errors.
presto-ui/src/d3utils.js
Outdated
Comment on lines
21
to
26
| export function initializeGraph() { | ||
| return new dagreD3.graphlib.Graph({ compound: true }) | ||
| .setGraph({ rankdir: "BT" }) | ||
| .setDefaultEdgeLabel(function () { | ||
| return {}; | ||
| }); |
Contributor
There was a problem hiding this comment.
nitpick: Return type annotation for initializeGraph is missing.
Please add a Flow return type annotation to initializeGraph for better type safety.
Contributor
Author
|
Sorry folks, accidentally committed some formatting changes... fix incoming shortly. |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-ui/src/package.json:59-60` </location>
<code_context>
+ "analyze": "webpack --env=production --config webpack.config.js --profile --json > stats.json && mv stats.json ../target/webapp/ && npx webpack-bundle-analyzer ../target/webapp/stats.json",
+ "lint": "eslint . --ext .js,.jsx",
+ "lint:fix": "eslint . --ext .js,.jsx --fix",
+ "format": "prettier .",
+ "format:fix": "prettier --write ."
},
"resolutions": {
</code_context>
<issue_to_address>
**suggestion:** Consider specifying file extensions for Prettier scripts to avoid formatting unintended files.
Using '.' as the target for Prettier may format files outside your intended scope. Limit formatting to specific extensions to avoid affecting non-source files.
```suggestion
"format": "prettier \"**/*.{js,jsx,json,css,md}\"",
"format:fix": "prettier --write \"**/*.{js,jsx,json,css,md}\""
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+59
to
+60
| "format": "prettier .", | ||
| "format:fix": "prettier --write ." |
Contributor
There was a problem hiding this comment.
suggestion: Consider specifying file extensions for Prettier scripts to avoid formatting unintended files.
Using '.' as the target for Prettier may format files outside your intended scope. Limit formatting to specific extensions to avoid affecting non-source files.
Suggested change
| "format": "prettier .", | |
| "format:fix": "prettier --write ." | |
| "format": "prettier \"**/*.{js,jsx,json,css,md}\"", | |
| "format:fix": "prettier --write \"**/*.{js,jsx,json,css,md}\"" |
Contributor
Author
|
OK, I'll have to remove the flow flags again, didn't realize it was all or nothing. It'll break prettier formatting for now, but will get through CI. I can complete flow annotations (or remove from files with minimal type annotations) in #25726 if that's acceptable. |
Member
|
Hi @johallar, thanks for the work. I am thinking separate the flow and prettier if that's possible. I believe prettier won't break the CI, however, it would contain a large change. (but we know it's just reformatting). And when you put the prettier change, you can also update the build to run prettier and make sure it is enforced afterwards. Then the other PR could only contain flow change, including enabling the flow annotation and fixing the errors. |
Contributor
Author
Yeah, I can split it that way, i'll plan to do 2 PRs then:
Let me know if you were thinking a different split @yhwang (eg. adding eslint + flow pieces in PR 1) |
Member
|
Yes, separating the flow from others is great! Let’s do it. |
Contributor
Author
Side note @yhwang @tdcmeehan: How committed is presto to flow? I know it's a meta library, but support + tooling is much better for typescript, as well as compatibility with 3rd party libraries. With how limited the flow usage is now in the presto UI I don't see the conversion being too hard, and much better to do now if that's something we might want to do going forward. |
Member
This is a good question, and I guess you probably could guess why flow is used at the Presto UI. And I totally understand your points and 100% agree with you. Here are my thoughts:
I believe wrapping up those PRs that refactor class components into function components would benefit us with TypeScript conversion. I am fine if you want to repurpose this PR to only apply ESLint and prettier, since we agreed to separate flow from ESLint. How do you think? |
Contributor
Author
|
Agree that eslint + ts is a great long term goal. If we're all in alignment on that I think we should do this in separate steps:
|
Member
|
Sounds like a great plan to me! |
Contributor
Author
|
Closing this one, replaced by #26281 |
7 tasks
yhwang
pushed a commit
that referenced
this pull request
Nov 18, 2025
## Description <!---Describe your changes in detail--> This removes flow, and replaces it with typescript, and is the next step in the plan laid out in #25716 (comment) ## Motivation and Context <!---Why is this change required? What problem does it solve?--> <!---If it fixes an open issue, please link to the issue here.--> Typescript has better tooling, and has a bigger community around it so this will be another QOL improvement for developers. Big # line changes is mostly yarn-lock changes, the code diff relatively small ## Impact <!---Describe any public API or user-facing feature change or any performance impact--> ## Test Plan <!---Please fill in how you tested your change--> CI passes, there should be no user facing changes ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
For developer + codebase life improvements, adding ESLint with prettier, react + react hooks, and flow plugins.
Motivation and Context
Impact
// @flowannotations to files that use flow so prettier/eslint know about itFollow up
Test Plan
For now this simply adds the dependencies, and implements an eslint/prettier configuration that works. No files have been linted or formatted yet. I plan to do this in a follow up to keep this PR manageable, first draft of linting all files here: #25726
Test the eslint config by running
yarn run lintfor eslint changes, oryarn run formatfor just prettier formatting changesContributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.