bpmn-io / bpmn-js

A BPMN 2.0 rendering toolkit and web modeler.
https://bpmn.io/toolkit/bpmn-js/
Other
8.55k stars 1.32k forks source link

Converting source code from functions to class #2242

Open mnp-mid opened 1 day ago

mnp-mid commented 1 day ago

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

export default class SubProcessPlaneBehavior extends CommandInterceptor {
  static $inject = ['canvas', 'eventBus', 'modeling', 'elementFactory', 'bpmnFactory', 'bpmnjs', 'elementRegistry'];

  constructor(canvas, eventBus, modeling, elementFactory, bpmnFactory, bpmnjs, elementRegistry) {
      super(eventBus)
  }
   ...
}

instead of

export default function SubProcessPlaneBehavior(
    canvas, eventBus, modeling,
    elementFactory, bpmnFactory, bpmnjs, elementRegistry) {
       CommandInterceptor.call(this, eventBus);

       ...
    }
    ...
}

SubProcessPlaneBehavior.$inject = ['canvas', 'eventBus', 'modeling', 'elementFactory', 'bpmnFactory', 'bpmnjs', 'elementRegistry'];

inherits(SubProcessPlaneBehavior, CommandInterceptor);

Describe the solution you'd like

image

nikku commented 1 day ago

@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 (BaseViewer).

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.

mnp-mid commented 1 day ago

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.

nikku commented 1 day ago

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.

This is a mis-understanding. TypeScript is not settled (as a tool), and the project will very unlikely ever become a TypeScript project, with .ts files. What is settled is typing in the codebase, and types generated for the benefits of our users.

We'll discuss internally, but as mentioned the costs very likely outweight the benefits.

barmac commented 1 day ago

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.

nikku commented 1 day ago

the main benefit will be in the reduced bundle size.

Which is negated as soon as you GZIP things.