Skip to content

Conversation

@mcyph
Copy link
Collaborator

@mcyph mcyph commented Apr 10, 2021

As per discussion at #8:

  • Refactored most of library code to ES9 with classes, static variables,
  • Added documentation and some type information from typed-mxgraph
  • Manually converted many of the base files in view/, shape/ to typescript
  • Partially refactored examples to use ES9 classes with next/react in separate subfolder/app to base lib

@jgadsden jgadsden requested review from coclav and jgadsden April 11, 2021 08:40
Copy link

@jgadsden jgadsden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request contains a large number of commits, which makes it very difficult to review this.

@mcyph
Copy link
Collaborator Author

mcyph commented Apr 11, 2021

This pull request contains a large number of commits, which makes it very difficult to review this.

Yeah sorry about this :/

I have effectively changed the style of almost the entire library codebase multiple times over, with conversions to ES9, TypeScript, and most documentation. It's obviously not ideal, though to be honest my assessment was because the codebase was so large (90k+ lines) not doing things in batch could have taken months, if not years, if a release was ever possible. That said, aside from style changes and removing of VML/IE support to my knowledge the behavior of the code hasn't changed significantly.

  • Because there weren't unit tests, I've been trying to check that the behavior of the examples matches the original versions of the software. Further tests on the older versions of the code could be done by running the examples unmodified, that way backwards compatibility could be improved.
  • As I didn't understand the codebase well enough, I wasn't in a position to write tests, however going forwards considering how to do this and whether unit tests can be done to make sure the new code performs in the same way could be planned. Some of the documentation didn't match the behavior of the code before I started refactoring, and I think it would need to be done prior to enforcing standards.
  • My goal was essentially that the code can get to a stage where it's easier to build on and make mistakes look more obvious - the steps to do this include using TypeScript in strict mode and putting the old function definitions alongside the new ones to allow faster checking that things work the way they should. I'm hoping from now on the changes would likely be more incremental and pull requests would be more feasible, as most of the major changes I wanted to make are complete.

Let me know whether you think any changes should be made or what action you think is most appropriate. I won't have time to do it for the time being, but my personal thoughts are merging to dev now, but planning to create some python scripts to easily compare the old functions with the new ones side-by-side on top of sorting the examples+documentation out could be one of the better solutions.

@jgadsden jgadsden self-requested a review April 13, 2021 20:58
Copy link

@jgadsden jgadsden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mcyph - I had a mad moment, and thought this was going to main branch. But of course it is going into the development branch as we discussed, so many apologies for asking for more work on something that is perfectly OK.
Approved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Code refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants