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.

Sign up for GitHub

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

build(log-viewer-webui-client): Switch from Webpack to Vite for bundling; Convert the codebase to TypeScript. #645

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
+4,090 −8,307
Open

build(log-viewer-webui-client): Switch from Webpack to Vite for bundling; Convert the codebase to TypeScript. #645

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
bb3f508
Convert log-viewer-webui/client codebase to TypeScript; Update its de…
junhaoliao Dec 26, 2024
ad34028
Add newline to EOF.
junhaoliao Dec 26, 2024
3444981
Rename `define-process-env-node-env` -> `config-node-env` in webpack…
junhaoliao Dec 26, 2024
8b03576
Switch from Webpack to Vite for client bundling.
junhaoliao Dec 29, 2024
4976d95
Update ESLint dependencies and configurations; lint code.
junhaoliao Dec 29, 2024
ff5da99
Fix indent in Taskfile.yml
junhaoliao Dec 29, 2024
7ac39ee
Refactor LOCAL_STORAGE_KEY to use TypeScript enum.
junhaoliao Dec 29, 2024
d0c1443
Enhance TypeScript strictness in tsconfig files.
junhaoliao Dec 29, 2024
a7998e1
Update dependencies in log-viewer-webui client
junhaoliao Jan 11, 2025
d6a224b
Refactor build commands with loop for modularity.
junhaoliao Jan 11, 2025
2c6e393
Add `--emptyOutDir` flag to the vite build command to get rid of the …
junhaoliao Jan 11, 2025
b222fc6
Correct typo in `QUERY_LOADING_STATE_VALUES` docstring - Apply sugges…
junhaoliao Jan 11, 2025
c3b26e2
Add comment to clarify the proxy target address.
junhaoliao Jan 11, 2025
0a3de9e
Update TypeScript configs with stricter linting.
junhaoliao Jan 12, 2025
a85679c
Move `EXTRACT_JOB_TYPE` to `typings/query.ts`. Add docs for `QUERY_JO…
junhaoliao Jan 12, 2025
ca86be9
Ensure root element exists before initializing React app.
junhaoliao Jan 12, 2025
2767de1
Rename and refactor LOCAL_STORAGE_KEY module.
junhaoliao Jan 12, 2025
c345e00
Refactor query parsing with type-safe validation by `@sinclair/typebox`.
junhaoliao Jan 12, 2025
8a29872
Clean up query parameter parsing.
junhaoliao Jan 12, 2025
e434b64
Install eslint-config-yscope @ v1.0.0 and fix linting violations.
junhaoliao Jan 13, 2025
1116797
Enforce "new-cap" ESLint rule and update code accordingly.
junhaoliao Jan 13, 2025
05a51e4
Update package.json to use npm ci for deterministic installs.
junhaoliao Jan 13, 2025
580b215
Update ESLint config to adjust capIsNewExceptions list.
junhaoliao Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions Taskfile.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,12 @@ tasks:
sources:
- "{{.G_BUILD_DIR}}/log-viewer-webui-node-modules.md5"
- "{{.TASKFILE}}"
- "client/index.html"
- "client/tsconfig.*.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

this would skip tsconfig.json. is it expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was intended as the tsconfig.json file was only meant to point references to the tsconfig.app.json and tsconfig.node.json files, but now I think about it again, we should add the common options into tsconfig.json so that we don't necessarily repeat in the other two files. Then we should be tracking client/tsconfig*.json instead.

- "client/package.json"
- "client/src/**/*.css"
- "client/src/**/*.jsx"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like in the past the linter doesn't run on *.js file, and no it will run on all *.ts file (which I believe to be the replacement for *.js)

This looks perfectly expected, but just want to double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, in the past it was a mistake not to include *.js files here.

- "client/src/webpack.config.js"
- "client/src/**/*.ts"
- "client/src/**/*.tsx"
- "yscope-log-viewer/package.json"
- "yscope-log-viewer/public/**/*"
- "yscope-log-viewer/src/**/*"
Expand All @@ -245,13 +247,14 @@ tasks:
DATA_DIR: "{{.OUTPUT_DIR}}"
cmds:
- "rm -rf '{{.OUTPUT_DIR}}'"
- for:
- "client"
- "yscope-log-viewer"
cmd: |-
cd "{{.ITEM}}"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--output-path "{{.OUTPUT_DIR}}/{{.ITEM}}"
- |-
cd "client"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--outDir "{{.OUTPUT_DIR}}/client"
- |-
cd "yscope-log-viewer"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--output-path "{{.OUTPUT_DIR}}/yscope-log-viewer"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix indentation in build commands

The build commands have inconsistent indentation which could cause YAML parsing issues.

Apply this diff to fix the indentation:

      - |-
        cd "client"
        PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
-        --outDir "{{.OUTPUT_DIR}}/client"
+          --outDir "{{.OUTPUT_DIR}}/client"
      - |-
        cd "yscope-log-viewer"
        PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
-        --output-path "{{.OUTPUT_DIR}}/yscope-log-viewer"
+          --output-path "{{.OUTPUT_DIR}}/yscope-log-viewer"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- |-
cd "client"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--outDir "{{.OUTPUT_DIR}}/client"
- |-
cd "yscope-log-viewer"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--output-path "{{.OUTPUT_DIR}}/yscope-log-viewer"
- |-
cd "client"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--outDir "{{.OUTPUT_DIR}}/client"
- |-
cd "yscope-log-viewer"
PATH="{{.G_NODEJS_22_BIN_DIR}}":$PATH npm run build -- \
--output-path "{{.OUTPUT_DIR}}/yscope-log-viewer"

- task: "utils:compute-checksum"
vars:
DATA_DIR: "{{.OUTPUT_DIR}}"
Expand Down
35 changes: 18 additions & 17 deletions ...log-viewer-webui/client/public/index.html → ...onents/log-viewer-webui/client/index.html
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

You should format this file, so indented. In VSCode, I used there default formatter and fixed it.

Copy link
Member Author

@junhaoliao junhaoliao Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This was already formatted with PyCharm's formatter, where they don't indent the first-level children in the <body/> tag. Let me know if you mean any other format changes are required.

In the future, we can also add Prettier / HTML-lint to Lint the html files

Copy link
Contributor

Choose a reason for hiding this comment

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

kk sounds good. Ignore

Original file line number Diff line number Diff line change
@@ -1,17 +1,18 @@
<!doctype html>
<html lang="en">
<head>
<title>Log Viewer</title>
<meta charset="utf-8"/>
<meta name="description" content="YScope Log Viewer">
<meta name="viewport" content="initial-scale=1, maximum-scale=1">
<link rel="preconnect" href="https://fonts.googleapis.com"/>
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin/>
<link rel="stylesheet"
href="https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&display=swap"
/>
</head>
<body>
<div id="root"></div>
</body>
</html>
<!doctype html>
<html lang="en">
<head>
<title>Log Viewer Web UI</title>
<meta charset="utf-8"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added " Web UI"

<meta name="description" content="YScope Log Viewer Web UI">
<meta name="viewport" content="width=device-width, initial-scale=1.0"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added " Web UI"

<link rel="preconnect" href="https://fonts.googleapis.com"/>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied from the Vite react-ts tempalte.

<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin/>
<link rel="stylesheet"
href="https://fonts.googleapis.com/css2?family=Inter:wght@300;400;500;600;700&display=swap"
/>
</head>
<body>
<div id="root"></div>
<script type="module" src="/src/main.tsx"></script>
</body>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied from the Vite react-ts tempalte.

</html>
Loading
Loading