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

deps(client): bump bpmn-io dependencies #3113

Merged
merged 1 commit into from
Sep 6, 2022
Merged

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Sep 5, 2022

  • @bpmn-o/properties-panel@0.20.0
  • bpmn-js@9.4.0
  • bpmn-js-properties-panel@1.6.0
  • bpmn-moddle@9.4.0
  • camunda-bpmn-js@0.17.0
  • camunda-bpmn-moddle@7.0.1
  • diagram-js@8.9.0
  • zeebe-bpmn-moddle@0.15.0

Closes #2994
Closes #3085

@pinussilvestrus
Copy link
Contributor Author

Something is going on with preact-markup : https://github.com/camunda/camunda-modeler/runs/8183836279?check_suite_focus=true#step:6:391

ERROR in ../node_modules/preact-markup/dist/preact-markup.module.js 1:0-42
Module not found: Error: Can't resolve 'preact' in '/home/runner/work/camunda-modeler/camunda-modeler/node_modules/preact-markup/dist'

@pinussilvestrus
Copy link
Contributor Author

@barmac do you maybe have an idea? Neither preact nor preact-markup were updated.

@barmac
Copy link
Collaborator

barmac commented Sep 5, 2022

I'll have a look at this.

@barmac
Copy link
Collaborator

barmac commented Sep 5, 2022

Let's re-run the tests first ⏳

@barmac
Copy link
Collaborator

barmac commented Sep 5, 2022

OK so it still fails. What is more, I can reproduce the issue on my machine.

Preact-markup peer-depends on preact and until now it used the preact installed via the form-viewer.

I noticed that on my machine preact-markup is eventually placed in the root node_modules while preact stays in client/node_modules. This may be the reason why it fails. Let's check if removing lerna bootstrap helps.

@barmac
Copy link
Collaborator

barmac commented Sep 5, 2022

It does not :(

We need to somehow make sure that preact-markup is installed in the same node_modules or a level below preact. That should fix the issue.

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Sep 5, 2022

We could add preact-markup as a dependency of client, that would fix the issue.

However, I wonder why this doesn't happen during install of bpmn-js-properties-panel. Probably related to the npm@8/lerna@5 upgrade?

@barmac
Copy link
Collaborator

barmac commented Sep 6, 2022

We could add preact-markup as a dependency of client, that would fix the issue.

However, I wonder why this doesn't happen during install of bpmn-js-properties-panel. Probably related to the npm@8/lerna@5 upgrade?

This is super weird, because we don't use --hoist 🤯 . Also, preact-markup is a client-only dependency, so there's no reason for the tool to hoist it.

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Sep 6, 2022

Fixed via 0f1d435. A reinstall seemed to do the trick.

This is ready to review now.

@pinussilvestrus pinussilvestrus marked this pull request as ready for review September 6, 2022 07:20
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Sep 6, 2022
@pinussilvestrus pinussilvestrus requested review from philippfromme and barmac and removed request for philippfromme September 6, 2022 07:20
* `@bpmn-o/properties-panel@0.20.0`
* `bpmn-js@9.4.0`
* `bpmn-js-properties-panel@1.6.0`
* `bpmn-moddle@9.4.0`
* `camunda-bpmn-js@0.17.0`
* `camunda-bpmn-moddle@7.0.1`
* `diagram-js@8.9.0`
* `zeebe-bpmn-moddle@0.15.0`

Closes #2994
Closes #3085
@pinussilvestrus
Copy link
Contributor Author

It seems like upgrading to npm@8 and lerna@5 led to some surprises when installing dependencies (also cf. bpmn-io/form-js#325 (comment)). I discussed this with @barmac and agreed to talk about this with the team in one of our next weeklies. ⚡

Copy link
Collaborator

@barmac barmac left a comment

Choose a reason for hiding this comment

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

I tested this locally and it works correctly. As a follow-up, we should also fix or look into the detected vulnerabilities: 33 vulnerabilities (3 low, 13 moderate, 16 high, 1 critical) .

@pinussilvestrus pinussilvestrus merged commit c65b04f into develop Sep 6, 2022
@pinussilvestrus pinussilvestrus deleted the bump-dependencies branch September 6, 2022 09:14
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 6, 2022
@pinussilvestrus
Copy link
Contributor Author

I will follow up with some audit clean-ups.

pinussilvestrus pushed a commit that referenced this pull request Sep 6, 2022
* Follow-up of #3113
* Fixes some audit errors
pinussilvestrus pushed a commit that referenced this pull request Sep 6, 2022
* Follow-up of #3113
* Fixes some audit errors
pinussilvestrus pushed a commit that referenced this pull request Sep 12, 2022
* Follow-up of #3113
* Fixes some audit errors
pinussilvestrus pushed a commit that referenced this pull request Sep 12, 2022
* Follow-up of #3113
* Fixes some audit errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Process ID Duplicated When Copying a Participant Warn users about hidden sequence flows
2 participants