-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Converting source code from functions to class #2242
Comments
@mnp-mid You mention the one benefit of this change: "folks used to ES6 (classes only) code base) can navigate the source code better". On the flip side the transformation will be a full rewrite of the code base and break any prior context established in pull requests and external links. It will also break with some usage patterns we currently have ( To my knowledge users can use classes on top of our function-style components (we do it). Bottom line: It does not feel worth it to me, at this stage. |
This change cannot be done overnight, of course, and breaking changes will occur the bigger the file is, as it brings more complexity in rewriting. It should work out fine for smaller files, I suppose. TypeScript is already partially settled in this project, so considering using classes would bring you closer to use TypeScript, which has better support for types and refactoring. My intention with this request is more likely to think through this and maybe put it on the roadmap. |
This is a mis-understanding. TypeScript is not settled (as a tool), and the project will very unlikely ever become a TypeScript project, with We'll discuss internally, but as mentioned the costs very likely outweight the benefits. |
I've had a look into this some time ago, and parts of our codebase cannot be easily converted to classes. One example is the unusual late call of the inherited class: https://github.com/bpmn-io/bpmn-js/blob/develop/lib/BaseViewer.js#L698. Also, any extension using pre-ES6 inheritance will need to be rewritten as this throws an error: class A {}
function B() { A.call(this) }
new B(); I think if we are to rewrite the pre-ES6 classes to modern JavaScript, the main benefit will be in the reduced bundle size. |
Which is negated as soon as you GZIP things. |
Is your feature request related to a problem? Please describe.
I sometimes go through the source code and try to understand how things are working or releated to each other. The code is build based on functions / prototype structure.
I wonder, if there are any plans or if the code could move on to class structure. This has the benefit of keeping things more together and in case you plan to move on to typescript with more compiling support, this would be the next step to consider. Of course this is some work to be done, but it could go step by step.
Something like
instead of
Describe the solution you'd like
The text was updated successfully, but these errors were encountered: