-
-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Migrate UI to Typescript and Vite #1908
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
feat: Migrate UI to Typescript and Vite #1908
Conversation
Thank you @joachimdalen I will try to test this in GITPOD to see if I can find some issues. @jcanizalez if you have some time maybe you can take a look for this PR. |
I was facing this when running the app in gitpod (It is a pretty good tool for quickly testing you could read this) I added the allowedHost like this inside the vite.config.mts: export default defineConfig(() => {
return {
server: {
port: 3000,
allowedHosts: ["3000-joachimdalen-terrakube-agdq4lnouvv.ws-us118.gitpod.io"]
},
build: {
outDir: "build",
rollupOptions: {
output: {
manualChunks: function (id) {
if (id.includes("react-icons")) return "icons";
},
},
},
},
plugins: [react(), commonjs()],
rollup: {
plugins: [dynamicImportVars()],
},
};
}); I guess the allowedHost should be like a parameter to start the app right? Now I am facing this We were using this URL with the parameters for the UI Right now the file is empty |
I'll take a look and see if I can find out what is going on |
From what I can see the code itself seems to work. The values are populated as long as the .env file / environment variables exists with the correct values. So it seems for some reason that environment variables is not being populated when the script that generates the config runs, which is weird as I've not touched any of that configuration 🤔 The I will need to do some further digging tomorrow and possibly try it in Gitpod |
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.
Pull Request Overview
This pull request refactors a significant portion of the UI by migrating from JavaScript to TypeScript and updating build tools to Vite. Key changes include adding TypeScript support with type safety improvements, updating ESLint configurations, and reorganizing component files while removing obsolete JavaScript versions and tests.
Reviewed Changes
Copilot reviewed 93 out of 100 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ui/eslint.config.js | Updated ESLint configuration to support TypeScript and React rules |
ui/src/domain/Home/MainMenu.tsx | Migrated MainMenu component to TypeScript and updated navigation behavior |
ui/src/config/authUser.ts | Converted authUser module to TypeScript |
ui/src/domain/Home/App.tsx | Adjusted routing and layout integration in the main application component |
ui/src/ActionLoader.tsx | Added TypeScript type annotations for dynamic imports and improved error handling |
ui/src/domain/Home/Home.tsx | Updated Home component with TypeScript annotations and minor formatting adjustments |
ui/src/config/axiosConfig.ts | Standardized axios configurations with improved formatting |
ui/src/config/authConfig.ts | Adjusted the auth configuration with TypeScript and refined error messages |
ui/src/config/actionTypes.js | Removed obsolete JavaScript version of action types |
ui/src/config/authUser.js | Removed obsolete JavaScript version of authUser |
ui/src/domain/Home/App.test.js | Removed outdated test file |
ui/src/domain/Home/MainMenu.jsx | Removed obsolete JavaScript version of MainMenu |
Files not reviewed (7)
- ui/.prettierrc: Language not supported
- ui/.vscode/settings.json: Language not supported
- ui/index.html: Language not supported
- ui/package.json: Language not supported
- ui/public/index.html: Language not supported
- ui/src/domain/Home/App.css: Language not supported
- ui/src/domain/Home/Home.css: Language not supported
Comments suppressed due to low confidence (1)
ui/src/domain/Home/App.test.js:1
- The removal of the App.test.js file reduces test coverage; please ensure that equivalent tests are implemented in the new TypeScript setup to verify core functionality.
import { render, screen } from '@testing-library/react';
I did the following changes inside the package.json
Looks like it is working after the above changes |
I do not fully understand why that solved it, but the important part is that the issue is solved. I'll update that in the change as well as look into parametrizing the |
So I've looked a bit into this. The easiest fix would probably be to use the It looks like this would only be the case for GitPod as it loads the development server. There is a lot going on with configuration, so I am a bit unsure about where / how to best add this so it is correct. Are you able to point me to the correct file that this should be configured in? |
Sure let me check the documentation that you shared and do some test on my side |
I was able to fix it like this @joachimdalen Gitpod execute this script when the workspace is starting: Line 59 in 46c9bd0
I added this two lines
After this line
The above updates the The environment variables file is used in the launch command for the UI I run the script again manually I started the UI And it is working Adding the above will make it easier to test your UI changes |
I need to test the VCS connections and workspace webhooks logic, but every other option in the UI seems to be working properly :) |
Great! I just did what should be the last changes, so I will publish this now. A note about linting of the files - Currently it's reporting a lot of errors:
This is something I think is better resolving as development moves further. Having looked through some of them they require changes to logic that I do not wish to do in this pull request. It is already large and complex enough. |
First of all, apologies for the massive pull request. The file count is a bit misleading due to renames/extension changes. I tried to make it smaller, however with changing both language and build utils at the same time it would have left some parts unfinished and confusing when compared to other parts of the UI code.
I'm creating this a draft to collect some feedback before publishing the pull request. There is still a lot of testing that needs to be done to ensure the refactor did not break anything. I'd appreciate some testing from you guys who knows the ins and outs of the project.
This will close #1904 once merged.
Why submit this before it is 100% finished?
I believe that early input and discussion is good for the best product outcome. Therefor I am creating this now to get input from the maintainers and other community members. I am not a frontend developer by trade, but have worked enough with frontends to know my way around them. Any input or constructive criticism is welcome.
What has changed?
The biggest change in this pull request is the change from JavaScript to Typescript. This introduces more type safety into the project and will hopefully make it easier for others to contribute as well. It also improves the experience when working with third party libraries.
To the best of my ability I have tried to create accurate types with their correct nullability by comparing code in both the UI and the API. With that said there is probably some mismatch that I hope to correct later on. I had to use more instances of
any
than I would have liked, but this is also something to correct later on. Some types are a bit verbose as the moment as multiple data structures of the same domain object is used in different parts. This will hopefully be resolved with some more refactoringOther changes
To be able to get the types as accurate as possible, I've flattened some objects that were used in different components. Mostly an extra
data
for arrays that was being saved to the state as an object. As a consequence of this, there is also some functional changes included in this pull request.VsCode works best for frontend projects when opened directly on the root (at least from my experience). Therefor I've created an additional
.vscode
folder with the appropriate configuration inside of/ui
.Some other fixes/changes done during this migration:
deleteWebhook
insettings/advanced
as it was being called without parameters and causing failuresWhat is left?
A summary of what is left before this is considered publish ready: