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

Question about branches and PR #21

Open
liusida opened this issue Jun 7, 2024 · 10 comments
Open

Question about branches and PR #21

liusida opened this issue Jun 7, 2024 · 10 comments

Comments

@liusida
Copy link

liusida commented Jun 7, 2024

Hi guys,

I saw that @daniel-lewis-ab is working on the reversion and @atlasan is working on forwardcompatible. And I've sumitted a PR to the master. Is master the branch I should PR to? Did I do it right? Or, should I join @atlasan to fix things on forwardcompatible?

I need some suggestion. Thanks!

@liusida
Copy link
Author

liusida commented Jun 7, 2024

And what does the phrase forward compatible mean? Does it mean that it has the latest featuers and will be merged to master progressively?

@atlasan
Copy link
Collaborator

atlasan commented Jun 8, 2024

Hi @liusida, good to see you here ^ _ ^
I chose the name for the temporary branch I'm working on. Surely the intention is to be compatible and forward looking ^ _ ^
Please feel free to join, need some hands in testing, cleaning, and improving.

@liusida
Copy link
Author

liusida commented Jun 8, 2024

Hi @liusida, good to see you here ^ _ ^ I chose the name for the temporary branch I'm working on. Surely the intention is to be compatible and forward looking ^ _ ^ Please feel free to join, need some hands in testing, cleaning, and improving.

Sounds like a plan. Do you have any specific tasks that I can work on? I can try one of them first.

@liusida
Copy link
Author

liusida commented Jun 8, 2024

And, what do you think about the PR #19, since that PR can make litegraph.js runnable in plain html, and we won't need to start a http server for the development any more. Should I PR to your branch instead of the master?

@liusida
Copy link
Author

liusida commented Jun 8, 2024

I've just learned the basics of jest. And I think I can do some testing to start with. What's your opinion towards using ChatGPT to accelerate the writing of test cases?

@atlasan
Copy link
Collaborator

atlasan commented Jun 8, 2024

Sounds like a plan. Do you have any specific tasks that I can work on? I can try one of them first.

I would say:

  • implementing PR fix the path for uglifyjs #19 seems good to me
  • think about the best way to allow different version of the library in ComfyUI ( I was imagining a setting in there where to specify the name of the folder/file to be included. This would help testing and switching to pre-release or custom UIs easily. eg. clone the repo in a folder and specify that folder name in the ComfyUI settings
  • test the library with built-in demos and with ComfyUI for potential issues (I will try to test with WebGlStudio too)
  • review the changes made since branching to see what I have been working on and detect problems or clean up

And many more:

  • review the implementation of CallbackHandlers and prototype simple extensions leveraging the system (like autoconnect or renamer) to debug and finish the implementation with many multiple callbacks
  • check and think about ComfyUI LiteGraph extensions: some should be implemented in LG using CallbackHandlers
  • check the list of implemented CallbackHandlers: update the docs, propose implementation of return value for many of those (when a callback would modify default behavior)
  • create demo workflow and implement those in the default editor, we can make a demo per functionality and some complex testing
  • rework and implement the node library provided in LG
  • check and finish new logging system implementation ( console methods have been replaced, in the code some old debug need to be fixed or new debugs are to be added using good standards and with correct level )

@atlasan
Copy link
Collaborator

atlasan commented Jun 8, 2024

I've just learned the basics of jest. And I think I can do some testing to start with. What's your opinion towards using ChatGPT to accelerate the writing of test cases?

Nothing against as long as nice and working ^_ ^

What about making workflows that are tests themselves? The way to check at high level the system could be to use it for real.
A complex stepped demo would eventually cover two interests.

@liusida
Copy link
Author

liusida commented Jun 8, 2024

making workflows that are tests themselves

What a brilliant suggestion~ xD

@daniel-lewis-ab
Copy link
Owner

daniel-lewis-ab commented Jun 8, 2024 via email

@liusida
Copy link
Author

liusida commented Jun 11, 2024

think about the best way to allow different version of the library in ComfyUI ( I was imagining a setting in there where to specify the name of the folder/file to be included. This would help testing and switching to pre-release or custom UIs easily. eg. clone the repo in a folder and specify that folder name in the ComfyUI settings

Yeah. I naively thought that compiling this project into a litegraph.core.js file and replace the original one in ComfyUI would automatically bring new features to ComfyUI. (I've actually tried, but it doesn't work...)

How do you do this manually @atlasan ? Can the current forwardcompatible branch be applied to current ComfyUI?

I'll start a new issue on this #22.

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

3 participants