-
Notifications
You must be signed in to change notification settings - Fork 448
Add support for Feature Flags #4439
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
This is to avoid having people muck with the values for now. We'll add the ability to modify feature flags (in a way that won't conflict with other custom nodes or future changes to core) later.
webfiltered
left a comment
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 please note the need for the new interfaces to be any typed and merge?
If there isn't one (anymore/yet?), can we make them unknown instead? It makes the eventual enabling of "no implicit/explicit any type" rules easier.
src/scripts/api.ts
Outdated
| /** | ||
| * Alias for serverFeatureFlags for test compatibility. | ||
| */ | ||
| get feature_flags() { | ||
| return this.serverFeatureFlags | ||
| } | ||
|
|
||
| set feature_flags(value: Record<string, any>) { | ||
| this.serverFeatureFlags = value | ||
| } |
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.
What does this do? If it's removed and the refs in the unit test replaced with serverFeatureFlags, everything still passes.
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've removed it. Claude suggested it and I assumed it was related to how TypeScript stubs out functionality in unit tests. Not really sure though now 🤔
Had no idea |
output.mp4A video showing that ComfyUI still works. |
webfiltered
left a comment
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.
The blurriness took me a second, but I managed to read your test prompt. Megapproved. 🤣
This change implements feature flag support on the frontend. When the frontend
establishes a websocket connection to the backend, it will send a feature flag message
informing the backend of available functionality. This allows the backend to be smarter
about sending messages that the frontend will understand.
We also receive a feature flag message from the backend and provide functions to query
available functionality on the backend. This allows custom nodes (and built-in code)
to gate functionality depending on whether the backend actually supports it.
This change was factored out of #4382
┆Issue is synchronized with this Notion page by Unito
A video showing that ComfyUI still works (no visible changes result from this PR.)
https://github.com/user-attachments/assets/d79b01f0-df5f-426c-b69a-b8a56e4a5453