Ri 5876 fix start electron dev mode #4017
Conversation
Initial working POC - to be cleaned
There was a problem hiding this comment.
Please clear this pull request. There are unused files, console.logs, if(.... true)
- There are changes for BE code, please rename branch to 'be/feature/...' to auto run BE tests
- check your eslint. For desktop folder should apply root .eslintrc.js. This code has a lot of eslint errors: import orders, semicolon etc
Backend has own eslint with semicolon.
redisinsight/desktop/forge.config.js
Outdated
| @@ -0,0 +1,61 @@ | |||
| const { FusesPlugin } = require('@electron-forge/plugin-fuses'); | |||
There was a problem hiding this comment.
Is this file forge.config.js used? I can't find a link to it anywhere.
There was a problem hiding this comment.
You are right. I've missed to clean it up after when I started removing all different experiments. Removed.
redisinsight/desktop/package.json
Outdated
| "build:renderer": "vite build --config vite.renderer.config.ts" | ||
| }, | ||
| "devDependencies": { | ||
| "@vitejs/plugin-react": "^4.3.3", |
There was a problem hiding this comment.
We should not have duplicates of dependendcies
Please remove all dependencies from this file. They should be taken from root package.json.
Or move dependencies from root to here.
There was a problem hiding this comment.
cleared the entire dependencies for the desktop. Tested to make sure it will still work.
package.json
Outdated
| "yarn-deduplicate": "^3.1.0" | ||
| }, | ||
| "dependencies": { | ||
| "@antv/hierarchy": "^0.6.14", |
There was a problem hiding this comment.
you don't need add dependencies.
Vite optimise warning appears if dependencies from the packages folder (redisinsight/ui/src/packages) have not been installed.
or you can exclude them in the ui vite.config.mjs
optimizeDeps: {
exclude: [
'react-json-tree',
'redisinsight-plugin-sdk',
'plotly.js-dist-min',
'@antv/x6',
'@antv/x6-react-shape',
'@antv/hierarchy'
],
There was a problem hiding this comment.
I am a bit confused, why shouldn't we add them as dependencies, if the warning is caused by them not being installed?
There was a problem hiding this comment.
They aren't used in the RedisInsight application.
They are a part of plugins (redisinsight/ui/src/packages). Each plugin is built separately from RedisInsight. Plugins dependencies are not used in the Redis Insight. Vite optimization doesn't see the difference and shows useless warning
There was a problem hiding this comment.
there is documentation about Plugins
https://github.com/RedisInsight/RedisInsight/wiki/Plugin-Documentation
There was a problem hiding this comment.
got it! THank you for the explanation. Cleared those as well.
| @@ -0,0 +1,53 @@ | |||
| import { defineConfig } from 'vite'; | |||
There was a problem hiding this comment.
Yes, in the corresponding package.json
| entryFileNames: '[name].js', | ||
| } | ||
| }, | ||
| target: 'node14', |
There was a problem hiding this comment.
most likely an artifact from testing. Removed
…com/RedisInsight/RedisInsight into RI-5876-Fix-start-electron-dev-mode
|
@zalenskiSofteq your feedback has been addressed, please take another look. About the lining, there are multiple lint errors for API and UI from before this PR. When are those typically enforced? Should we set up the husky hooks for that? |
To set up husky we need to fix all the errors, it's gonna take time. |
Steps to start (from the root folder each in a different browser tab):
yarn dev:electron:uiyarn dev:electron:apiyarn dev:electronThis will start the fe and be with the required environments for electron as well as the electron app.
Live reload works both for the ui and api.
There is a new added script -
dev:apifor the sake of consistency. Right now we had to run (based on the wiki)yarn dev:uiandyarn --cwd redisinsight/api/ start:devwhich seems difficult to remember and inconsistent.Running just
yarn dev:uiand yarndev:apiin separate terminals (or terminal tabs) will still allow the project to be ran in a browser (no loss of functionality).All of the changes are aimed for development environment, production builds should be unaffected (tested on local linux builds)
Major changes
Added vite configs for the desktop folder to keep it consistent
Decoupled the desktop from loading parts of the api to handle window authentication and cloud authentication. On dev now those are done through TCP servers. This is what allows for everything to be ran separately and still have live/hot reload. This is also going to be used only during development
2.1 All windows have the window id header in the requests and are checked in the api through the TCP communication approach
2.2 The renderer is able to handle click on a social login button, then the main process is able to ping the api, the api is able to generate a URL, pass it back, open it in the default browser, complete the flow and then have the protocol of redisinsight:// be opened as a result. Everything going through the TCP communication instead of the desktop directly invoking methods of the api.
Added 4 different packages that I've had yarn complain are not installed but required on 2 different OSs
I've tried to keep the changes as minimal as possible.
Any and all feedback is welcomed (especially if I've missed another place the desktop relied on the api in such a way).