Skip to content
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

[Bug]: Issues importing types from @comfyorg/comfyui-frontend-types: types not exported #2083

Open
pterrien opened this issue Dec 28, 2024 · 5 comments
Assignees

Comments

@pterrien
Copy link

pterrien commented Dec 28, 2024

Frontend Version

v1.5.19 (but not relevant here). NPM package @comfyorg/comfyui-frontend-types v1.6.8

Expected Behavior

When importing package @comfyorg/comfyui-frontend-types in my own Comfy TS project, expecting to be able to import types.

Actual Behavior

Just an example with InputSpec: if I try to import it to add proper typings to my function, I get the following:
Module '"@comfyorg/comfyui-frontend-types"' declares 'InputSpec' locally, but it is not exported.

Steps to Reproduce

In a TS file, just add import { InputSpec } from '@comfyorg/comfyui-frontend-types'

Debug Logs

N/A

Browser Logs

N/A

Setting JSON

N/A

What browsers do you use to access the UI ?

Mozilla Firefox

Other

Hello,

Not 100% here is the right place to post this, please tell me otherwise.

As stated above, I am trying to convert my soon-to-be-published custom node repo to Typescript.
To do so, I have installed both NPM packages @comfyorg/litegraph and @comfyorg/comfyui-frontend-types.
I didn't face issues importing from the former, but do get many like the one above with the latter.

When I inspect the code from this repo, I see that InputSpec is exported (in src/types/apiTypes.ts, line 319):

export type InputSpec = z.infer<typeof zInputSpec>

But when I check the types from NPM package, type is not exported (@comfyorg/comfyui-frontend-types/index.d.ts, line 1009):

declare type InputSpec = z.infer<typeof zInputSpec>;

Same with many other types: ComfyWidgetConstructor, ...

Did I miss something, and could you kindly guide me?
One fact I find surprising: out of 33,000 lines in @comfyorg/comfyui-frontend-types/index.d.ts, the export keyword is only present 4 times, for: ComfyApi (interface + class), ComfyApp, ComfyExtension.
Plus a strange empty export { } at the very bottom. Possibly an issue with bundler?

Thanks in advance for your help, and once again sorry if it's not the right place to post about it.

┆Issue is synchronized with this Notion page by Unito

@pterrien pterrien added the Potential Bug Untriaged bug label Dec 28, 2024
@pterrien
Copy link
Author

Inspecting a bit further, I see that in src/types/index.ts, only ComfyApi, ComfyApp and ComfyExtension are indeed exported.
Does this mean that we are not supposed to use this package for anything else than those 3 types?

@huchenlei
Copy link
Member

Inspecting a bit further, I see that in src/types/index.ts, only ComfyApi, ComfyApp and ComfyExtension are indeed exported. Does this mean that we are not supposed to use this package for anything else than those 3 types?

What is your use case for InputSpec? Based on my observation most frontend extensions only use ComfyApi, ComfyApp and ComfyExtension so only these 3 types are exported.

@huchenlei huchenlei removed the Potential Bug Untriaged bug label Dec 28, 2024
@pterrien
Copy link
Author

Hi,

Thank you for your quick answer, much appreciated!
As I understand it (please correct me otherwise), this type is useful when registering new custom widgets types in getCustomWidgets(), inside app.registerExtension() (here is a JS version):

getCustomWidgets(app) {
    return {
        MY_CUSTOM_TYPE(node, inputName, inputData, app) {
            // I guess inputData is of type InputSpec here?
        }
    }
}

Am I correct?
My point was to ensure I properly use the native typings as much as possible.

Beyond that, I must admit: I also need a few other type imports that are not covered and probably not useful for most people: $el, .... And I fully understand the NPM package is meant for the largest audience, not edge cases.
So in the meantime, I investigated this current repo config, and found it quite easy to generate the typings I need, by adapting the build:types script, and copying the .d.ts file inside my own repo. It's not the cleanest, but it's probably acceptable as it's just for typings after all.

So, as a summary:

  1. thank you for your answer!
  2. maybe InputSpec (and a few other types) could be useful to people, it's up to you to decide; otherwise I will deal with it manually
  3. for the rest ($el), I consider that's certainly out-of-scope, and will deal with it in my own way, not a big deal

@huchenlei
Copy link
Member

Hmmm, I should probably test it out myself by writing an example extension with the type library. Thanks for flagging that out!

@huchenlei huchenlei self-assigned this Dec 28, 2024
@pterrien
Copy link
Author

While completing my migration, I also encountered 2 other small issues:

  1. Similarly, not exported: ComfyNodeDef, used for nodeData in beforeRegisterNodeDef() inside app.registerExtension, defined in src/types/comfy.ts (line 105):
  beforeRegisterNodeDef?(
    nodeType: typeof LGraphNode,
    nodeData: ComfyNodeDef,
    app: ComfyApp
  ): Promise<void> | void
  1. The widely used app.registerExtension() is marked as deprecated, with the message: "Use useExtensionService().registerExtension instead". However, useExtensionService is not exported, too.
    Of course, I guess we can continue using app.registerExtension for a while though.

I thought you might be interested in these ones, too.

Hope this helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants