search

jagenjo / litegraph.js

A graph node engine and editor written in Javascript similar to PD or UDK Blueprints, comes with its own editor in HTML5 Canvas2D. The engine can run client side or server side using Node. It allows to export graphs as JSONs to be included in applications independently.
MIT License
5.33k stars 602 forks source link

Conversion to ES6 Classes (Stable, Major) #455

Closed daniel-lewis-ab closed 1 month ago

daniel-lewis-ab commented 2 months ago

This PR is to pull:

I made as few modifications to the code beyond that as possible, and each one was made as a separate commit.

jagenjo commented 2 months ago

Hey, can you restore the TS files? they are important to other people.

daniel-lewis-ab commented 2 months ago

Hi Javi,

I put the litegraph.d.ts file back into the folder.

My background: I'm familiar with python servers, know a little of node.js, apt, pip, venv, an online beautifier, asm, C, D, JS, Python, CSS, HTML5, web design and development, restful interfaces, comfyui, AI models, llama-cpp-python, LLM and neural network theory, etc.

If you want a restoration (like restoring an old axe) done in TS, maybe put me in touch with someone familiar with more of your toolchain who wants to work together on it - I presume you aren't that invested anymore.

But I was going to focus on fixing the events infrastructure, separate concerns a bit so it was more maintainable, and then start extracting features from HTML Canvas to HTML+SVG.

I can be reached on Discord as mr_pebble, or on Element as @mr_pebble:matrix.org @.***_pebble:matrix.org>

Regards, Daniel

On Thu., Mar. 7, 2024, 02:46 Javi Agenjo, @.***> wrote:

Hey, can you restore the TS files? they are important to other people.

— Reply to this email directly, view it on GitHub https://github.com/jagenjo/litegraph.js/pull/455#issuecomment-1983122038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC36LN2XUWRZ4GI3DROOUWTYXAZPRAVCNFSM6AAAAABEJTY7GOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBTGEZDEMBTHA . You are receiving this because you authored the thread.Message ID: @.***>

moritz89 commented 2 months ago

Regarding the TS file, we use litegraph.js in our Angular project (TS) and would be open to help if anything specific comes up. From a technical side, we mainly use the presentation layer of litegraph.js and execute the graph in a separate engine.

Currently we mainly focus on packaging a release after every MR so that they are easily used from NPM: https://www.npmjs.com/package/@inamata-co/litegraph.js

daniel-lewis-ab commented 2 months ago

Hi Moritz,

Nice to e-meet you. To catch you up in case it helps:

https://github.com/jagenjo/litegraph.js/pull/455

I basically wrote a large part of an update for LiteGraph.js in JS from 2011 to ES6 and am continuing to work on the next step. My intent was to have a functioning modern flow graph library, but I don't currently use TS for anything, don't use Angular, and don't use npm.

In the interest of LiteGraph serving everyone properly, it does seem to be appropriate to support the variety of uses of it in ways that are practicable, but I myself am writing this as a matter of hobby and I'm not sure I'm committed to learning a new language and toolchain to do so given that fact. So I asked Javi to put me in touch with someone I could discuss this with who could fill the role of working with me to support that side of things.

If that isn't you, that's fine too, but do you happen to know anyone who does seem like a good fit for that?

Regards, Daniel

My background:

I'm familiar with python servers, know a little of node.js, apt, pip, venv, an online beautifier, asm, C, D, JS, Python, CSS, HTML5, web design and development, restful interfaces, comfyui, AI models, llama-cpp-python, LLM and neural network theory, etc.

If you want a restoration (like restoring an old axe) done in TS, maybe put me in touch with someone familiar with more of your toolchain who wants to work together on it - I presume you aren't that invested anymore.

But I was going to focus on fixing the events infrastructure, separate concerns a bit so it was more maintainable, and then start extracting features from HTML Canvas to HTML+SVG.

I can be reached on Discord as mr_pebble, or on Element as @mr_pebble:matrix.org @.***_pebble:matrix.org>

On Thu, Mar 7, 2024 at 11:06 AM Moritz Ulmer @.***> wrote:

Regarding the TS file, we use litegraph.js in our Angular project (TS) and would be open to help if anything specific comes up. From a technical side, we mainly use the presentation layer of litegraph.js and execute the graph in a separate engine.

Currently we mainly focus on packaging a release after every MR so that they are easily used from NPM: @.***/litegraph.js

— Reply to this email directly, view it on GitHub https://github.com/jagenjo/litegraph.js/pull/455#issuecomment-1984138495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC36LN5HMLOP5TUC73XSTZDYXCUAPAVCNFSM6AAAAABEJTY7GOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBUGEZTQNBZGU . You are receiving this because you authored the thread.Message ID: @.***>

moritz89 commented 2 months ago

The TS and npm functionality / work can be built on top of your changes without you requiring to interact with it. In order to use JS code in TS, its tooling and Angular, only the type definition .d.ts files have to be written. I'd gladly keep these up.-to-date with changes to the JS files. Regarding the npm releases, I use an automated approach that only requires running a single command.

A workflow that could work is soon after an MR is accepted, me and my team would update the .d.ts file if required and then publish a release. This would mirror the existing work that I do to publish npm releases.

The only question I'd have is whether I should continue publishing in our npm package (@inamata-co/litegraph.js) or to publish in Javi's original npm package (litegraph.js)?

jagenjo commented 2 months ago

Hi all:

I think this is a nice PR and I want to accept it, as it is always nice to clear the code a little. My only concern is that this is more than a simple refactor, some features are dissapearing, which is a no-go as people depend on them. I created a release with the current version so we can talk about changes. For instance, I wont accept moving to SVG, mostly because that closes the door to many cool features (like rendering the graph using WebGL, which Im using in other projects). I will leave this PR for a while, testing it, and if it seems to work fine, I will accept it. In the future it will be good to keep PRs smaller, as this one is scary, but I appreciate the effort.

Cheers

daniel-lewis-ab commented 2 months ago

Javi,

Yeah, I didn't see a smaller cut-off to perform the transfer to ES6 without converting the whole thing given the dependencies and connections and such. I do appreciate it was a rather huge step, and my next steps are all quite a lot smaller.

I do hope we can effect changes to this so none or very little of the features are lost before/as/shortly-after it becomes main-line.

In particular, the utils, npm, TS, etc. were already mentioned.

My next PR will then be some minor patching to classes to get various things more right.

You are welcome to suggest particular changes you'd like to see.

On Fri, Mar 8, 2024 at 1:53 AM Javi Agenjo @.***> wrote:

Hi all:

I think this is a nice PR and I want to accept it, as it is always nice to clear the code a little. My only concern is that this is more than a simple refactor, some features are dissapearing, which is a no-go as people depend on them. I created a release with the current version so we can talk about changes. For instance, I wont accept moving to SVG, mostly because that closes the door to many cool features (like rendering the graph using WebGL, which Im using in other projects). I will leave this PR for a while, testing it, and if it seems to work fine, I will accept it. In the future it will be good to keep PRs smaller, as this one is scary, but I appreciate the effort.

Cheers

— Reply to this email directly, view it on GitHub https://github.com/jagenjo/litegraph.js/pull/455#issuecomment-1985296574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC36LN5GHV5AIVZ5AIGDYVDYXF4CNAVCNFSM6AAAAABEJTY7GOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBVGI4TMNJXGQ . You are receiving this because you authored the thread.Message ID: @.***>

daniel-lewis-ab commented 2 months ago

Javi,

I just realized the PR is continuing to take updates from my main branch, rather than the branch I intended it to come off of. It is only intended to be up to https://github.com/jagenjo/litegraph.js/pull/455/commits/f195a96b95ae4abddb5a506216cf15c7d39ba6e0

daniel-lewis-ab commented 2 months ago

I have this frozen and will not push another change until I have more instructions. I can rewind it, leave it, finish, whatever you like Javi.

daniel-lewis-ab commented 1 month ago

https://github.com/jagenjo/litegraph.js/pull/462