Skip to content
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

Open
mnp-mid opened this issue Oct 1, 2024 · 5 comments
Open

Converting source code from functions to class #2242

mnp-mid opened this issue Oct 1, 2024 · 5 comments
Labels
backlog Queued in backlog enhancement New feature or request

Comments

@mnp-mid
Copy link

mnp-mid commented Oct 1, 2024

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

@mnp-mid mnp-mid added the enhancement New feature or request label Oct 1, 2024
@nikku
Copy link
Member

nikku commented Oct 1, 2024

@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
Copy link
Author

mnp-mid commented Oct 1, 2024

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
Copy link
Member

nikku commented Oct 1, 2024

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.

@nikku nikku added the backlog Queued in backlog label Oct 1, 2024 — with bpmn-io-tasks
@barmac
Copy link
Member

barmac commented Oct 1, 2024

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
Copy link
Member

nikku commented Oct 1, 2024

the main benefit will be in the reduced bundle size.

Which is negated as soon as you GZIP things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Queued in backlog enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants