-
Notifications
You must be signed in to change notification settings - Fork 199
Converts JS files to TS. #42
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
Conversation
- Converting JS files to TS.
|
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 |
|
@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 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 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 |
|
@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 |
|
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 |
|
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 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. |
|
It's nice to hear the works don't overlap :) |
|
Small status update - still highly experimental but I've merged your changes and have been trying to separate out the main 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. |
|
@mcyph How about pushing your changes(even if it's unfinished) here and let's work together on it? |
|
Sure! I've pushed my changes to a new branch at https://github.com/maxGraph/maxGraph/tree/mcyph-graph-refactor |
|
FYI, @mcyph and I are working on mcyph-graph-refactor branch mentioned above which has all the changes in this PR. So, I'll close this PR. Thanks! |
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!