bpmn-io / bpmn-js-differ

A diffing utility for BPMN 2.0 documents.
MIT License
45 stars 17 forks source link

feat: Add support for diffing changes to timer events. Added tests to verify #19

Closed Gawdfrey closed 2 months ago

Gawdfrey commented 4 months ago

Proposed Changes

closes #15

The library has missed support for diffing changes made to bpmn:TimerEventDefinition. This PR adds support for this, by including bpmn:TimerEventDefinition in the list of tracked definitions. The solution is in whole taken from this comment.

Steps to test

  1. Pull down repository
  2. Run npm install
  3. Run npm run all
  4. See that all tests passes and that the results object from the timer event looks something like this:
    ChangeHandler {
    _layoutChanged: {},
    _changed: { TimerEventDefinition_0fgktse: { model: [Base], attrs: {} } },
    _removed: {},
    _added: {}
    }

Checklist

To ensure you provided everything we need to look at your PR:

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

barmac commented 3 months ago

Hi,

Thanks for your contribution. I see that it's not ready for review yet given your comment https://github.com/bpmn-io/bpmn-js-differ/pull/19#discussion_r1643397380

Do you want to finish this pull request? If so, please tag me in a comment. Otherwise we will close it if there are no updates soon.

Gawdfrey commented 3 months ago

Hi @barmac, been on holiday the last weeks. Will look into the regression this week :)

nikku commented 2 months ago

@Gawdfrey Thanks for your contribution.

This looks very tailored towards "timer event definition". What about any kind of change, affecting any kind of event definition? How would such a solution look like? Is there a good reason to only support timer events?

nikku commented 2 months ago

@Gawdfrey I thank you again for your contribution! As commented previously (https://github.com/bpmn-io/bpmn-js-differ/pull/19#issuecomment-2296502852) this, unfortunately, is a fix tailored to timers (only). We cannot generalize it easily to be applicable to all sorts of events (and, in the context of technical execution, extension elements). Hence I'm closing your PR.

We hope to get to the linked issue back at some point, and implement a fix.