Skip to content

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.

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

Closed
dmx974 wants to merge 26 commits into from
Closed

Improving new stack and prepare. #218

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
db3665b
(update) gitignore
dmx974 Jul 2, 2024
30812e5
Merge branch 'Comfy-Org:main' into main
dmx974 Jul 6, 2024
fb02b87
Merge branch 'main' of github.com:dmx974/ComfyUI_frontend
dmx974 Jul 25, 2024
6b02f6d
(fix) api for crystools.monitor
dmx974 Jul 25, 2024
fab3d35
(update) prettier and code format
dmx974 Jul 25, 2024
efcd6ae
(update) CSS formatting
dmx974 Jul 25, 2024
6c1f240
(update) add components.d.ts to gitignore
dmx974 Jul 25, 2024
97222be
(update) add package-lock.json to gitignore
dmx974 Jul 25, 2024
3f8128a
(add) config file
dmx974 Jul 25, 2024
026ac21
(add) unplugin, vueuse, iconify, vuerouter
dmx974 Jul 25, 2024
05b4d0d
(update) remove crystools message from websocket handler
dmx974 Jul 25, 2024
ca5eaf0
(update) tailwind config
dmx974 Jul 25, 2024
3b2cc59
(add) spinner component
dmx974 Jul 25, 2024
fb3f2da
(add) loader component
dmx974 Jul 25, 2024
d66e4b1
(add) default and graph layouts
dmx974 Jul 25, 2024
579dc9a
(update) extract system nodeDefs from nodeDefStore
dmx974 Jul 25, 2024
1c8f721
(update) use config file to get app version instead of window
dmx974 Jul 25, 2024
8c2fd7d
(add) BlockUI component
dmx974 Jul 25, 2024
e207e45
(add) in house VueConfirm
dmx974 Jul 25, 2024
d5a6712
(add) in house VueToast
dmx974 Jul 25, 2024
8c16ca1
(add) vue headlessui
dmx974 Jul 25, 2024
cbc2c76
(fix) tailwind CSS not working
dmx974 Jul 25, 2024
de32bd5
(fix) SideToolBar
dmx974 Jul 25, 2024
f603a92
(add) mainStore, prepare for progress bar
dmx974 Jul 25, 2024
4a1ab6a
(remove) icons and unused CSS
dmx974 Jul 25, 2024
13dfe7e
(update)
dmx974 Jul 25, 2024
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
4 changes: 4 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default {
Copy link
Contributor

@huchenlei huchenlei Jul 25, 2024

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.

Copy link
Contributor

@huchenlei huchenlei Jul 25, 2024

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.

Copy link
Contributor Author

@dmx974 dmx974 Jul 26, 2024

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).

image
image

What we want at the end is just the LiteGraph, and everything else around made with reusable Vue components, like this:

image

Copy link
Contributor

@huchenlei huchenlei Jul 26, 2024

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.

Copy link
Member

@robinjhuang robinjhuang Jul 26, 2024

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 👍

// @ts-ignore
app_version: __COMFYUI_FRONTEND_VERSION__
}