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

refactor: [POC] Replace mxGraph with maxGraph 0.10.3 #3098

Closed
wants to merge 97 commits into from

Conversation

tbouffard
Copy link
Member

@tbouffard tbouffard commented May 27, 2024

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 #2756
Covers #3238

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 e8fa907 (0.37.0 with dev dependencies update only)

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

Main fixes included in this POC

Done

  • maxGraph: most problems detected in 0.1.0 are fixed in 0.10.3 (implementation, types)
  • SVG Exporter is now working thanks to fixes in https://github.com/maxGraph/maxGraph/releases/tag/v0.10.3
  • most e2e tests are now passing. There are small discrepancies on rendering of dashed strokes. This may be due to an unwanted change in maxGraph during the migration from mxGraph

To be continued in next POC

  • some TODO relates to elements that are already fixed in maxGraph 0.10.3 (workaround, extra configuration, ...). They have not been implemented because they require stable e2e tests passing prior the changes to ensure the changes do not introduced regression
  • maxGraph now only provides ESM files, so the Jest tests running the maxGraph API have been updated to use ESM (unit and integration tests). e2e and performance tests have been updated as well, but some refactoring are still needed
    • Some shared configuration files have been duplicated with an implementation for CommonJS (formely used everywhere) and one for ESM
    • the attachment of images in the e2e tests results have been disabled: it failed after the move to ESM config
    • the whole test configuration must be reviewed/fixed/improved in the future pocs
  • tests results
    • unit: 100%
    • integration: 100%
    • e2e: Test Suites: 7 failed, 3 passed, 10 total / Tests: 61 failed, 193 passed, 254 total

Bundle size comparison

https://github.com/maxGraph/maxGraph/releases/tag/v0.6.0 should reduce the size significantly comparing to the previous POC (see #2366 (comment)).

Bundle maxGraph 4.2.2 maxGraph 0.10.1 maxGraph 0.10.2 (with extra code to debug SvgExporter) maxGraph 0.10.3
IIFE raw 1 669 729 - - 1 249 348
IIFE minified 984 186 - - 604 940
ESM 198 486 - - 194 144
Demo chunk 818 KB 465.93 kB 476.12 kB 466.12 kB

Notes:

  • the demo chunk: the demo involves additional mxGraph/maxGraph code for the SVG Exporter (it uses the ImageExport API). As a result, this chunk is larger than the chunk in an application integrating bpmn visualization in a standard way.
  • the ESM bundle: api-extractor removes all comments from the code before generating this bundle, so the size of the bundle is a good indicator of the impact of migration on the amount of code needed to interact with with mxGraph or maxGraph.

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:

Planned but not done:

  • run unit and integration tests: configure Jest to run with ESM.
    • maxGraph now only provides ESM files
    • Initially, this would have removed 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).
    • lodash is no longer used, and its replacement name toolkit-es provides a CommonJS entry point. So this is no longer needed in the master branch. See chore(deps): switch from lodash-es to es-toolkit #3142

Detected issues in maxGraph

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

  1. Redo a POC with the latest available maxGraph release (0.15.0 on 2025-02-06)
  2. Poc the migration on a more recent version of the codebase as e8fa907 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 added 10 commits July 4, 2023 06:53
BpmnGraph: remove batchUpdate as it is already defined in the Graph class

GraphCellUpdater.ts: WIP fix TSC errors

StyleManager.ts: WIP fix TSC errors

StyleComputer.ts: WIP fix TSC errors

BpmnRenderer.ts: update TODO

StyleConfigurator.ts: WIP fix TSC errors

StyleConfigurator.ts: add TODO after rebase

ShapeConfigurator.ts: WIP fix TSC errors

fix rebase: fix FONT imports (no more exported in style/index - internal only)

Add TODO in BPMNCellStyle

style/utils.ts: WIP fix TSC errors

StyleManager.ts: remove unused imports

StyleComputer.test.ts: WIP fix TSC errors

integration tests: WIP fix TSC errors

mxGraph.model.bpmn.elements.test.ts: restore variable name after rebase (no mx in names)

run fix CSS class undefined access - WIP CSS classes disapaer when doing navigation

fix typo in TODO/FIXME about maxgraph

StyleComputer.test.ts: fix msg flow icon tests after rebase

StyleComputer.test.ts: fix sub-process tests after rebase

StyleComputer.test.ts: clean comment in sub-process tests after rebase

the comments was the test implementation in the previous poc. During rebase, as there were too many conflicts, the tests
 hadn't been migrated and the former poc implem put there to help the new test migration

StyleComputer.test.ts: fix subprocess markers test

StyleComputer.test.ts: fix event gateway test

StyleComputer.test.ts: WIP fix bpmn in color

StyleComputer.ts: fix msg icon flow extension computation

StyleComputer.ts: improve comment

StyleComputer.ts: fix edges extension computation

style-utils.test.ts: fix subprocess tests (adhoc still KO)

style-utils.test.ts: fix msg flow icon

style-utils.test.ts: FIXME for adhoc

ThemedBpmnVisualization.ts: fix wrongly migrated comment

refactor: restore getDataModel method when getModel was called with mxGraph

refactor: restore usage of getDataModel method in BpmnRenderer.ts

refactor: restore usage of getDataModel method in GraphCellUpdater.ts

GraphCellUpdater.ts add TODO for the master branch

SvgExporter.ts: restore import order like with mxGraph imple

ShapeConfigurator.ts: restore import order like with mxGraph implem

ShapeConfigurator.ts: restore comment

StyleConfigurator.ts: restore import order

StyleConfigurator.ts: add todo for arc configuration

custom-overlay.ts: restore import order

toBeShape/index.ts: fix compilation errors

toBeEdge/index.ts: fix compilation errors

matcher-utils.ts: fix compilation errors

mxGraph.model.bpmn.elements.test.ts: fix compilation errors

integration tests: fix CSS api tests

mxGraph.model.css.api.test.ts: check no extra css classes prior adding them

IT: simplify check of extra css classes edge/shape

WIP debug extra CSS classes not added to the dom

style-utils.test.ts: fix test

WIP restore style.isInitiating: make test pass

WIP restore style.isInitiating

integration test: BpmnCellStyle.horizontal is now a boolean (same type as in CellStyle)

integration test: fix test in "bpmn in color"

StyleComputer.ts remove TODO about "isInitiating" (already managed)

mxGraph.model.bpmn.elements.test.ts: revert extra changes

StyleComputer.ts remove TODO about "style.horizontal" (already managed)

StyleComputer.ts fix label position (horizontal and vertical)

style-utils.test.ts: remove TODO already managed

StyleComputer.ts: simplify management of style.horizontal for lane and pool

update TODO

custom-overlay.ts: restore original license header

model-expect.ts: remove extra imports

model-expect.ts: simplify import

WIP tmp fix to update the style

WIP tmp fix to update the style and CSS classes (clone the style of the cell to cache and update)

refactor: simply the storage of CSS classes in the BPMN style object

integ test: refactor markers management

integ test: restore the implem of the getFontStyleValue utils function (make 2 tests pass)

CSS classes: only set in style when the array is not empty (fix 10 integration tests)

TMP: more logs in the matcher error msg to better diagnose the problem

WIP integ test: investigate lower fontStyle value

WIP integ test: fix fontStyle value

update TODO comments for consistency

fix more integration tests: CSS class reset

fix wrong assertion in IT: fontStyle

updateStyle fontStyle: less side effect when setting flag of undefined value

StyleComputer: do not set fontStyle value to 0 (restore mxGraph behaviour) + fix IT

IT: remove extra check

fix: updateStyle 'default' special value now correctly reset the value

style utils.ts: improve TODO

improve comments about workarounds for maxGraph issues

Add TODO set plugins in Graph constructor

mxGraph.model.style.api.test.ts remove extra FIXME

mxGraph.model.style.api.test.ts add TODO

StyleComputer.ts remove managed TODO

style utils: update todo

test: simplify todo

remove managed TODO

matcher-utils.ts: remove tmp solution to jest msg

matcher-utils.ts remove managed TODO

Manage "todo rebase"

update todo

fix style for rounded activity (was not correcly migrated)

StyleConfigurator.ts: migrated code better fit the original code

manage some "todo rebase"

refactor: style function - no need to return the style object anymore

refactor: rename style properties used to fill markers (prepare future feature available in maxGraph)

Add todo about fixing "fill edge markers"
@tbouffard tbouffard added refactoring Code refactoring poc 💫 Experimentation to discuss about future implementation. Not intended to be merged mxgraph integration Something involving mxGraph (be aware of EOL) labels May 27, 2024
…SS in master branch)

WIP integration tests run in ESM (with commonJS configuration)

Revert "EXTRA WIP run integration in ESM mode"

This reverts commit 21c3691647630cd11f5572789d47cc5a22decfce.

EXTRA WIP run integration in ESM mode

EXTRA WIP run integration in ESM mode (vscode config)
with commonJS configuration

EXTRA WIP unit tests run in ESM (with commonJS configuration)

WIP unit tests run in ESM (with commonJS configuration)

WIP unit tests run in ESM (with commonJS configuration) test pass

WIP unit tests run in ESM (with commonJS configuration) vscode config
The promises management was wrong, images were not correctly assigned to the right file
@tbouffard tbouffard force-pushed the poc/bump_maxgraph_0.10.0_v1 branch from 8942a9b to f50b06e Compare August 13, 2024 15:18
@tbouffard tbouffard changed the title refactor: [POC] Replace mxGraph with maxGraph 0.10.x refactor: [POC] Replace mxGraph with maxGraph 0.10.3 Feb 4, 2025
@tbouffard
Copy link
Member Author

The POC is now completed, so closing this PR.

@tbouffard tbouffard closed this Feb 8, 2025
@tbouffard tbouffard deleted the poc/bump_maxgraph_0.10.0_v1 branch February 8, 2025 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mxgraph integration Something involving mxGraph (be aware of EOL) poc 💫 Experimentation to discuss about future implementation. Not intended to be merged refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant