search

camunda / camunda-bpmn-js

Embeddable Camunda modeling distributions based on bpmn-js
MIT License
104 stars 22 forks source link

`bpmn-js-properties-panel` should be a normal "dependency" #113

Open boris-petrov opened 2 years ago

boris-petrov commented 2 years ago

Describe the Bug

I'm not sure if I'm doing everything right but just adding "camunda-bpmn-js": "0.13.0-alpha.8", to package.json and trying out to use the Modeler fails because bpmn-js-properties-panel is missing. It is added as a peer dependency which means that it won't be installed by yarn. I think it should be moved to dependencies.

Steps to Reproduce Expected Behavior Environment

P.S. The same applies to @bpmn-io/properties-panel.

barmac commented 2 years ago

Hi,

thanks for creating this issue. I will try to explain why both the @bpmn-io/properties-panel and bpmn-js-properties-panel need to be peer dependencies. If there's any flaw in my reasoning, please feel free to point it out so that we can achieve right solution.

To start with, we want developers using camunda-bpmn-js to be able to provide properties panel extensions in their custom builds. Also, we don't want require developers to use non-standard deduplication tools, e.g. npm dedupe.

At first, we included both projects as direct dependencies. This resulted in a structure of node_modules similar to one below:

├── camunda-bpmn-js
│   ├── node_modules
│   │   ├── bpmn-js-properties-panel
│   │   │   ├── node_modules
│   │   │   │   ├── @bpmn-io/properties-panel

When one added bpmn-js-properties-panel to the project dependencies in order to import useService hook, it would then look like this:

├── bpmn-js-properties-panel
│   ├── node_modules
│   │   ├── @bpmn-io/properties-panel
├── camunda-bpmn-js
│   ├── node_modules
│   │   ├── bpmn-js-properties-panel
│   │   │   ├── node_modules
│   │   │   │   ├── @bpmn-io/properties-panel

Because useService relies on preact context, it would not work in such a setup. The properties panel would use the nested context while custom extension would take the closest module which resulted in an error. The same applied to @bpmn-io/properties-panel. This is what we have experienced.

With peer dependencies, the node_modules looks like this:

├── @bpmn-io/properties-panel
├── bpmn-js-properties-panel
├── camunda-bpmn-js

So we have only one instance of each dependencies which rely on context and hooks. Thus, extensibility is maintained.

If you have a better idea how to achieve this, we are happy to receive suggestions.

boris-petrov commented 2 years ago

@barmac - thank you for the detailed answer. I was asking if this was connected with the other issue and you linked it while I was typing. :)

As for a better idea... I definitely don't have one. :smile: This is a very "hacky" way of making sure that the same preact context is used but I'm totally unfamiliar with both preact and writing plugins for bpmn-js so I really can't say more. I do understand your pain though and understand that this is the best you can do for now so thank you for the information!

If you would like, let's leave the issue open so maybe someone someday has a solution in mind. :)

philippfromme commented 2 years ago

Stupid question: Why not make preact a peer dependency of all these packages? 🤔

barmac commented 2 years ago

The reason we decided against it is that if we make preact dependency, we force the library users to install a preact version specified in the peer dependencies. We don't want to enforce preact version for the code which is not related to the properties panel.