-
Notifications
You must be signed in to change notification settings - Fork 470
Improving new stack and prepare. #218
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
Conversation
dmx974
commented
Jul 25, 2024
- add router, views, layout management
- add dynamic icon management and insertion (https://icones.js.org/collection/all) (test page /test)
- separate logic in small components
- LiteGraph / Canvas integration now as a Vue component (instead of direct DOM manipulation)
- refactor and simplify index.html, App.vue and main.ts
- prepare for header notifications, footer, left and right panes
|
I am going to break down this PR into smaller several smaller PRs. and review them there. |
Is there any specific consideration on ignoring package.lock? |
| @@ -0,0 +1,4 @@ | |||
| export default { | |||
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.
ComfyUI version is binded on window already. Binding on window allows wider access by extension developers.
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 am also happy to bind it to app to avoid further contaminate the window namespace.
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.
We don't want wider access by extension developers, but secure access. That's exactly the whole point of what we are trying to do here: removing the unnecessary and unrestricted access to the window and our DOM from extension developers. This is insane in terms of security and complexity. I will provide a complete SDK to handle this, where everything will be sandboxed in an iframe. The frontend will then expose an API and a communication protocol through a SDK. Anyone who wants to create a plugin or extension will have to adhere to strict specifications and use the SDK. Look at what Telegram is doing with their Mini Apps concept; this is where we want to go.
We are getting closer and closer to a clean front stack and codebase, but there is still a giant mess inside /src/scripts/ui. This all needs to be removed for good and converted to reusable Vue components. No direct DOM manipulation should be allowed for creating or removing elements, modals, and no access to our JS scope for plugins, extensions etc. We will provide authors with a secure and well-designed SDK and specifications so they can build plugins, extensions (and even full featured apps) on top of Comfy.
The approach I suggest is the following:
- Stop maintaining the mess in /src/scripts, and definitely do not spend too much time to continue to convert it to TypeScript as it would only increase complexity and make things worst (more verbose TS mess on top of existing JS mess = more mess at the end, more complexity)
- Leave all the vanilla JS mess as is for now to keep a certain (temporary) level of compatibility with the existing ecosystem, and take the time to convert the features one by one to Vue. I'm mainly talking here about the settings modal and the floating main menu (with the buttons Queue Prompt, Save, Load, Refresh, etc). It's unimaginable the complexity and the incredible amount of unnecessary Vanilla JS code here just to display a simple modal, four buttons and make a few HTTP requests to the backend. This has to stop.
- Once everything is converted (what I already started doing with SDFX), only LiteGraph should remain, along with our core features in high-level, simple, and reusable components, and the SDK for authors.
All our components should be like this: short, sandboxed with clear separation of concerns, easy to read and reusable:
<template>
<div
ref="canvasContainerRef"
class="graph-canvas-container flex w-full h-full relative overflow-hidden"
>
<canvas ref="canvasRef" id="graph-canvas" class="w-full h-full" />
</div>
</template>
<script setup lang="ts">
import { ref, onMounted } from 'vue'
import { app as comfyApp } from '@/scripts/app'
import { useMainStore } from '@/stores'
const emit = defineEmits(['ready'])
const mainStore = useMainStore()
const canvasRef = ref<HTMLCanvasElement | null>(null)
const canvasContainerRef = ref<HTMLDivElement | null>(null)
onMounted(async () => {
mainStore.spinner(true)
await comfyApp.setup(canvasRef.value)
mainStore.spinner(false)
emit('ready')
})
</script>
Trying to maintain long-term compatibility with the old way of doing things (extensions that directly access the window scope and the app object) will lead us straight to disaster, increasing technical debt, code complexity and the number of hacks (e.g., teleports in divs with fixed IDs of elements created directly in the DOM bypassing Vue, for example).
For instance, we should create a Vue modal by starting with only 2 settings, bind these settings to pinia correctly (in a proper settings store), once it's done and working, we remove those 2 settings from the legacy UI ($el...) modal. Repeat until the legacy settings modal can be totally removed. Also arrange the settings in tabs, like I've done on the screenshots below (no page scrolling, clear visible Submit call to action button on the bottom of the modal).
What we want at the end is just the LiteGraph, and everything else around made with reusable Vue components, like this:
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.
Stop maintaining the mess in /src/scripts, and definitely do not spend too much time to continue to convert it to TypeScript as it would only increase complexity and make things worst (more verbose TS mess on top of existing JS mess = more mess at the end, more complexity)
Leave all the vanilla JS mess as is for now to keep a certain (temporary) level of compatibility with the existing ecosystem, and take the time to convert the features one by one to Vue. I'm mainly talking here about the settings modal and the floating main menu (with the buttons Queue Prompt, Save, Load, Refresh, etc). It's unimaginable the complexity and the incredible amount of unnecessary Vanilla JS code here just to display a simple modal, four buttons and make a few HTTP requests to the backend. This has to stop.
Once everything is converted (what I already started doing with SDFX), only LiteGraph should remain, along with our core features in high-level, simple, and reusable components, and the SDK for authors.
I agree with the approach. In fact we only did minimal work converting the legacy js code to ts. The legacy features should be migrated to Vue one by one. However, I think how this migration should be done need some further discussion.
Here are some of my concerns:
- We are using a component library (PrimeVue) to avoid building wheels from scratch. PrimeVue already offers solution to progress spinner / toast / confirmation / progress bar. We should only introduce our own low level UI component when using PrimeVue's component is not possible for some extreme customization cases.
- The main store is directly moved from SDFX, but the states are left unused. Can we break it down and do it in a more progressive manner? I don't see any point adding a right panel state field there when we don't even have a right panel yet.
- As you said we should migrate features one by one, I think the PR should also be organized by feature. You can refer to v1.2.0 Side Bar & Menu rework #189 to see how the Queue migration was worked out. Ideally we don't want some unused dependencies and code floating there.
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.
Yeah migrating feature by feature sounds good 👍
| @@ -1,3 +1,7 @@ | |||
| @tailwind base; | |||
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.
If I add the tailwind init in the root, The default menu seems to be affected by the tailwind style as well, which is not what we wanted.
How did you managed to scope the tailwind effect to make browsertests pass?
src/assets/css/style.css
Outdated
| overflow: hidden; | ||
| grid-template-columns: auto 1fr auto; | ||
| grid-template-rows: auto 1fr auto; | ||
| background-color: var(--bg-color); |
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.
Can you justify these CSS changes? The first 4 rows are mapped to tailwindcss classes, but other ones are either modified (background and text color changes to not follow css var) or deleted.
| display: flex; | ||
| } | ||
|
|
||
| #graph-canvas-container { |
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.
Same here. Reason for removal.
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.
Please check my fork to understand why:
https://github.com/dmx974/ComfyUI_frontend/blob/main/src/views/graph/GraphCanvas.vue
|
Closing as most valuable changes are already cherry-picked and merged. |


