bpmn-visualization-js icon indicating copy to clipboard operation
bpmn-visualization-js copied to clipboard

refactor: [POC] Replace mxGraph with maxGraph 0.10.x

Open tbouffard opened this issue 8 months ago • 0 comments

Overview

This is a POC and it is not intended to be merged into master, so don't expect to find a high quality code here! 😺 This is a continuation of #2376

Please don't rebase on master. If necessary, merge the master branch but instead we will create a new POC with the latest changes from the master branch.

Start from master e8fa9071 (0.37.0 with dev dependencies update only)

  • this is the same commit as in #2376
  • the purpose is to apply a newer version of maxGraph to the same codebase as in the previous poc to see if most problems detected at that time are now fixed

Purpose

  • evaluate the migration effort
  • detect problems early
  • apply improvements to the master branch to ease the future migration and improve tests
  • provide feedback to the maxGraph community and if possible fixes that we integrated in this PR as workaround
  • be ready to easily reuse this effort in a new Pull Request to test new maxGraph releases

Status

TODO

  • recompute bundle size reduction
  • https://github.com/maxGraph/maxGraph/releases/tag/v0.6.0 should reduce the size significantly
  • information about the latest POC with maxgraph 0.1.0 are available in https://github.com/process-analytics/bpmn-visualization-js/pull/2366#issuecomment-1334983736
  • we could compare the new reduction with the former one

Main fixes included in this POC

Done

  • maxGraph: most problems detected in 0.1.0 are fixed in 0.10.0

Pending: SVG Exporter

Changes to apply to the master branch

The following are already applied to the branch of the POC and must be apply to master to prepare the future:

  • run unit and integration tests: configure Jest to run with ESM.
    • maxGraph now only provides ESM files
    • will remove a workaround in integration we setup in the past for lodash (we use the ESM flavor, so running Jest in CommonJS required to replace loadash-es by loadash to only have CommonJS lib).
  • e2e tests: fix the reports to correctly stored the "received" image when storing it in dedicated subfolder (mainly for navigation tests). The received path was not correctly computed --> #3145

Detected issues in maxGraph

  • https://github.com/maxGraph/maxGraph/pull/396
  • https://github.com/maxGraph/maxGraph/pull/423
  • https://github.com/maxGraph/maxGraph/pull/425
  • https://github.com/maxGraph/maxGraph/pull/426. Seen as a regression of #185, but was a configuration issue

With the demo, access to not available images http://localhost:10001/dev/public/elements-identification.html?url=/test/fixtures/bpmn/theme/01.most.bpmn.types.without.label.bpmn --> http://localhost:10001/dev/public/expanded.gif [HTTP/1.1 404 Not Found

Next steps

Poc the migration on a more recent version of the codebase as e8fa9071 is dated (Jul 4, 2023) and a lot of changes (mainly refactoring) has been done. These changes will require to apply the migration to new parts of the code.

Notes

N/A

tbouffard avatar May 27 '24 14:05 tbouffard