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

Conversion to ES6 Classes (Stable, Major) #455

Closed
wants to merge 146 commits into from

Conversation

daniel-lewis-ab
Copy link

This PR is to pull:

  • Split apart litegraph.js file into multiple class-oriented files.
  • Converted to ES6 classes for all of core and nodes.
  • Ran code through a pretty-printer, verifying no modifications to content were made.
  • Made several HTML and CSS validation improvements.
  • Identified a uninitialized variable bug.
  • Commented out SillyClient class in nodes\network.js and in HTML
  • Updated README.md's etc.
  • Updated utils/ build lists to reflect multiple class files of current state.

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

@jagenjo
Copy link
Owner

jagenjo commented Mar 7, 2024

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

@daniel-lewis-ab
Copy link
Author

daniel-lewis-ab commented Mar 7, 2024 via email

@moritz89
Copy link
Contributor

moritz89 commented Mar 7, 2024

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
Copy link
Author

daniel-lewis-ab commented Mar 7, 2024 via email

@moritz89
Copy link
Contributor

moritz89 commented Mar 7, 2024

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
Copy link
Owner

jagenjo commented Mar 8, 2024

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
Copy link
Author

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

@daniel-lewis-ab
Copy link
Author

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 f195a96

@daniel-lewis-ab
Copy link
Author

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
Copy link
Author

#462

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

Successfully merging this pull request may close these issues.

None yet

3 participants