-
Notifications
You must be signed in to change notification settings - Fork 82
feat(webui): Integrate Monaco editor for future Presto SQL query input. #1108
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
Changes from 10 commits
9ca2d54
b501260
e27d62e
99095de
bdfe59d
bf9fae1
f4757af
0d3eba3
57de1a1
89fe75e
e3dc4a4
b6eb73a
6d8398a
126782e
4660e42
0b745e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import {useEffect} from "react"; | ||
|
||
import { | ||
Editor, | ||
EditorProps, | ||
useMonaco, | ||
} from "@monaco-editor/react"; | ||
import {language as sqlLanguage} from "monaco-editor/esm/vs/basic-languages/sql/sql.js"; | ||
|
||
import "./monaco-config"; | ||
|
||
|
||
type SqlEditorProps = Omit<EditorProps, "language">; | ||
|
||
/** | ||
* Monaco editor with highlighting and autocomplete for SQL syntax. | ||
* | ||
* @param props | ||
* @return | ||
*/ | ||
const SqlEditor = (props: SqlEditorProps) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a fix for this with css. so now fixed |
||
const monaco = useMonaco(); | ||
|
||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
useEffect(() => { | ||
if (null === monaco) { | ||
return () => { | ||
}; | ||
} | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
// Adds autocomplete suggestions for SQL keywords on editor load | ||
const provider = monaco.languages.registerCompletionItemProvider("sql", { | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
provideCompletionItems: (model, position) => { | ||
const word = model.getWordUntilPosition(position); | ||
const range = { | ||
startLineNumber: position.lineNumber, | ||
endLineNumber: position.lineNumber, | ||
startColumn: word.startColumn, | ||
endColumn: word.endColumn, | ||
}; | ||
const suggestions = sqlLanguage.keywords.map((keyword: string) => ({ | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
detail: "SQL Keyword", | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
insertText: `${keyword} `, | ||
kind: monaco.languages.CompletionItemKind.Keyword, | ||
label: keyword, | ||
range: range, | ||
})); | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return {suggestions: suggestions}; | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
}, | ||
}); | ||
|
||
return () => { | ||
provider.dispose(); | ||
}; | ||
}, [monaco]); | ||
|
||
return ( | ||
<Editor | ||
language={"sql"} | ||
|
||
// Use white background while loading (default is grey) so transition to editor with | ||
// white background is less jarring. | ||
loading={<div style={{backgroundColor: "white", height: "100%", width: "100%"}}/>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (just an idea - not strictly needed in this PR) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i tried it already! it looks worse since the grey lines in the skeleton appear and disapear. the plain white box just looks better |
||
options={{ | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fontSize: 14, | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
lineNumbers: "on", | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
lineNumbersMinChars: 2, | ||
minimap: {enabled: false}, | ||
overviewRulerBorder: false, | ||
placeholder: "Enter your SQL query", | ||
scrollBeyondLastLine: false, | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
wordWrap: "on", | ||
}} | ||
{...props}/> | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
}; | ||
|
||
export default SqlEditor; |
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* eslint-disable import/default */ | ||
import {loader} from "@monaco-editor/react"; | ||
import * as monaco from 'monaco-editor'; | ||
import EditorWorker from "monaco-editor/esm/vs/editor/editor.worker?worker"; | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
self.MonacoEnvironment = { | ||
getWorker() { | ||
return new EditorWorker(); | ||
}, | ||
}; | ||
|
||
loader.config({monaco}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
declare module "monaco-editor/esm/vs/basic-languages/sql/sql.js" { | ||
import {languages} from "monaco-editor/esm/vs/editor/editor.api"; | ||
|
||
|
||
interface SqlLanguageDefinition extends languages.IMonarchLanguage { | ||
keywords: string[]; | ||
} | ||
davemarco marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export const language: SqlLanguageDefinition; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import {CaretRightOutlined} from "@ant-design/icons"; | ||
import { | ||
Button, | ||
Tooltip, | ||
} from "antd"; | ||
|
||
import useSearchStore from "../../../SearchState/index"; | ||
|
||
|
||
/** | ||
* Renders a button to run the SQL query. | ||
* | ||
* @return | ||
*/ | ||
const RunButton = () => { | ||
const queryString = useSearchStore((state) => state.queryString); | ||
|
||
const isQueryStringEmpty = "" === queryString; | ||
const tooltipTitle = isQueryStringEmpty ? | ||
"Enter SQL query to run" : | ||
""; | ||
|
||
return ( | ||
<Tooltip title={tooltipTitle}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a consistent look, we should disable this button when the querystring is empty, just like we did with the submit button. |
||
<Button | ||
color={"green"} | ||
disabled={isQueryStringEmpty} | ||
icon={<CaretRightOutlined/>} | ||
size={"large"} | ||
variant={"solid"} | ||
> | ||
Run | ||
</Button> | ||
</Tooltip> | ||
); | ||
}; | ||
|
||
export default RunButton; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
/* Allows the editor to shrink when page width decreases */ | ||
.input { | ||
width: 100%; | ||
min-width: 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import {useCallback} from "react"; | ||
|
||
import SqlEditor from "../../../../../components/SqlEditor"; | ||
import useSearchStore from "../../../SearchState/index"; | ||
import styles from "./index.module.css"; | ||
|
||
|
||
/** | ||
* Renders SQL query input. | ||
* | ||
* @return | ||
*/ | ||
const SqlQueryInput = () => { | ||
const handleChange = useCallback((value: string | undefined) => { | ||
const {updateQueryString} = useSearchStore.getState(); | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
updateQueryString(value || ""); | ||
}, []); | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<div className={styles["input"] || ""}> | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<SqlEditor | ||
height={"150px"} | ||
hoophalab marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
onChange={handleChange}/> | ||
</div> | ||
); | ||
}; | ||
|
||
export default SqlQueryInput; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
import { | ||
CLP_QUERY_ENGINES, | ||
CLP_STORAGE_ENGINES, | ||
SETTINGS_QUERY_ENGINE, | ||
SETTINGS_STORAGE_ENGINE, | ||
} from "../../../config"; | ||
import Dataset from "./Dataset"; | ||
import styles from "./index.module.css"; | ||
import RunButton from "./Presto/RunButton"; | ||
import SqlQueryInput from "./Presto/SqlQueryInput"; | ||
import QueryInput from "./QueryInput"; | ||
import SearchButton from "./SearchButton"; | ||
import TimeRangeInput from "./TimeRangeInput"; | ||
|
@@ -27,10 +31,21 @@ const SearchControls = () => { | |
return ( | ||
<form onSubmit={handleSubmit}> | ||
<div className={styles["searchControlsContainer"]}> | ||
{CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} | ||
<QueryInput/> | ||
<TimeRangeInput/> | ||
<SearchButton/> | ||
{SETTINGS_QUERY_ENGINE === CLP_QUERY_ENGINES.NATIVE ? | ||
( | ||
<> | ||
{CLP_STORAGE_ENGINES.CLP_S === SETTINGS_STORAGE_ENGINE && <Dataset/>} | ||
<QueryInput/> | ||
<TimeRangeInput/> | ||
<SearchButton/> | ||
</> | ||
) : | ||
( | ||
<> | ||
<SqlQueryInput/> | ||
<RunButton/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it at first, but it will complicate searchQueryStatus. Like right now searchQueryStatus is a separate component under SearchControls. I think it looks weird to have searchQueryStatus under the button. I though about putting searchQueryStatus next to the button, but it will just be mess to render searchQueryStatus in different locations for "presto" and "native". I thought just putting the button on the right makes "native" and "presto" have similiar layout simplifying the codebase |
||
</> | ||
)} | ||
hoophalab marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</div> | ||
</form> | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the bundle size after adding Monaco?
One reason we avoided using this
@monaco-editor/react
was that the bundle size increases significantly as all Monaco assets were packed into the bundle. Is it still the case today?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is seems reasonable here? https://bundlephobia.com/package/@monaco-editor/react@4.7.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i ran
npm run build
and this is the output:previously, it was
so we can see the package size increase is not significant possibly because only the
@monaco-editor/react
are included. however, i wonder where themonaco-editor
assets are addedUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default, the loader-config loads Monaco assets from a CDN (on my end it uses https://cdn.jsdelivr.net/npm/monaco-editor): https://github.com/suren-atoyan/monaco-react/blob/master/README.md#loader-config
This would mean that if a user accesses the webui without access to the Internet or behind a country / corporate managed firewall, they would not be able to use the search page.
can we follow https://github.com/suren-atoyan/monaco-react/blob/master/README.md#use-monaco-editor-as-an-npm-package to config the monaco assets imports? (there's a Vite specific section)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on my end, if i just use their Vite example loader code, the bundle size bumped from 2.5MB to 15MB.
i think the issue is with
if we use https://github.com/y-scope/yscope-log-viewer/blob/main/src/components/Editor/MonacoInstance/bootstrap.ts as a reference and import only what we need, the bundle size increase should be less than 972.2KB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually never mind. we do need the whole bundle for the autocomplete to work. If you only want the editor, then we can't have autocomplete i think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wont work since it will break the autocomplete, unless you find another thing to import that can fix. I was able to fix syntax highlighting but not autocomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding this may fix, but not sure why import "monaco-editor/esm/vs/editor/editor.all.js"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now understand this 972.2KB is counting sizes of assets only in the "min" bundle and does not include any ESM APIs. Since we use the ESM APIs, we do need to import differently (more).
Let's see the comment at "monaco-config.ts" to reduce the scope of import. In the future, we should support lazy loading those.