Skip to content

Conversation

@junsikshim
Copy link
Collaborator

  • Exports each constants in mxConstants.
  • Converts JS files to TS.

This is not yet complete, but I'm afraid someone might be doing the same work as me. To dodge confusion, and maybe work on this PR together, I'll keep this PR open.

I'll request for reviews when it's ready. Thanks!

@jgadsden
Copy link

jgadsden commented May 14, 2021

Hello @junsikshim, are we sure that we should do this? Is it worth raising an issue for this so that it can be discussed?

If the issue is raised then we can assign it to you, and that should solve the duplicated effort concern

@junsikshim
Copy link
Collaborator Author

@jgadsden I thought we were going to use TS given @mcyph has converted a few files already. If we need further discussions about it, I'd be happy to.

Btw, I have very limited time until the end of May, so I won't be able to make further progress in this PR until then. :(

@mcyph
Copy link
Collaborator

mcyph commented May 15, 2021

@jgadsden the changes we've already made are actually quite substantial. It was my opinion that it wasn't viable to build on and maintain mxGraph (or what it will be called in the future) going forwards unless there were significant changes.

A majority of the work which we've done so far in the development branch has been conversion to TypeScript, migration to JSDoc from Natural Docs, conversion of JavaScript prototypes to ES6 classes, style changes, simplification of the examples/reducing the need for making "in-place" global changes, and architectural changes intended to simplify things for new users. Pretty much all of the view/model parts (which are what is most depended upon by other modules) have already been converted in the development branch.

For a comparison between the previous and new code example style:

And a comparison of the changes in the core library code:

Many methods using mxCell or mxCellState had the same variable names throughout the code base prior to these changes, and I don't think it would have been feasible to do the significant architectural changes (that we're already a substantial way through completing) without converting to TypeScript. Strictly enforcing and annotating types makes it clearer what input parameters are required and what types of return values are possible, and will highlight a large percentage of cases where making changes will introduce unintended consequences.

Unfortunately I won't be able to make changes until the end of May either due to limited time, and there are a number of known bugs which I was intending to fix as a result of the changes, but this will take some time.

With reference to Threat Dragon: I would suggest if you'd like to use mxGraph that using the current main branch would probably be the best option as it is the most stable and tested option. However, I could assist you with migration to the new version once the development branch becomes more stable (if that's what you'd like to do, and you think that would help).

@mcyph
Copy link
Collaborator

mcyph commented May 15, 2021

@jgadsden Because the changes we've made are breaking changes, and previous code bases will need to rely on the previous version, I increasingly feel like it may be best to maintain "maxGraph legacy" (or whatever we'd like to call it) at the same time as separately developing the new version with updated style. This is something which I think should be discussed, either as an issue or in the discussions tab.

I think we should merge @junsikshim's changes either way though - the development branch has already been substantially converted to TypeScript after all, and this PR doesn't represent a significant change to this branch.

@jgadsden jgadsden mentioned this pull request May 15, 2021
@jgadsden jgadsden linked an issue May 15, 2021 that may be closed by this pull request
@jgadsden jgadsden requested a review from mcyph May 15, 2021 08:56
@jgadsden
Copy link

jgadsden commented May 15, 2021

Hello @mcyph and @junsikshim I have raised issue #44 so that this work is assigned to you both. This should avoid anybody else duplicating work that you are both in the middle of.

And apologies, I did not mean to suggest that we should not do this work, just that it be visible and agreed that this is what we are doing, which it is now for sure

@mcyph
Copy link
Collaborator

mcyph commented May 15, 2021

No worries @jgadsden, I've actually been thinking for a while how long/whether we support users of the currently unsupported version is something that should be addressed so will ask as soon as I get a minute. I'm thinking also, that it may be difficult for people who haven't been following the issues, discussions and pull requests to know exactly what has been changed in the development branch so it would be good to write a canonical document on the wiki or in the repo itself.

As both @junsikshim and I won't be able to work on this for the time being, and I haven't done any work that may conflict, I'll leave this open for now, but I've had a cursory look through the changes and they look ok to me+don't see any obvious mistakes or have much to suggest at the moment.

@junsikshim
Copy link
Collaborator Author

It's nice to hear the works don't overlap :)
The current code in this PR contains many compiler(typescript) errors, so I'll update the PR description when I resolve them.

@mcyph
Copy link
Collaborator

mcyph commented Jun 6, 2021

Small status update - still highly experimental but I've merged your changes and have been trying to separate out the main mxGraph class to allow for more tree-shaking+make things more manageable: https://github.com/mcyph/mxgraph/tree/master/packages/core/src/view

It doesn't compile yet but am getting there gradually - these last 2 days are the first chance I've had to work on maxGraph in the last few weeks.

@junsikshim
Copy link
Collaborator Author

@mcyph How about pushing your changes(even if it's unfinished) here and let's work together on it?

@mcyph
Copy link
Collaborator

mcyph commented Jun 13, 2021

Sure! I've pushed my changes to a new branch at https://github.com/maxGraph/maxGraph/tree/mcyph-graph-refactor

@junsikshim
Copy link
Collaborator Author

FYI, @mcyph and I are working on mcyph-graph-refactor branch mentioned above which has all the changes in this PR.
It currently has too many compiler errors, so I think it's better to work on it on a separate branch for now. When the storybook examples can actually be run it'd be a good time to merge into develop branch and look for any side-effects of refactoring.

So, I'll close this PR. Thanks!

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.

Convert JS files to TS

3 participants