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] Remove json2typescript dependency #443

Merged
merged 7 commits into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/architecture/bpmn-parsing.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ You can see the implementation in *BpmnXmlParser.ts*.

==== Json Parser

To parse a JSON data, we use the library https://github.com/AppVision-GmbH/json2typescript[json2typescript].
To parse a JSON data, we use a custom parser.

You can see the implementation in *BpmnJsonParser.ts*.
We create our custom _converters_ (in the *converter* folder where the *BpmnJsonParser* file is) in order to match the BPMN model to our internal model.
We have different _converters_ (in the *converter* folder where the *BpmnJsonParser* file is) in order to match the BPMN model to our internal model.
5 changes: 0 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@
"dependencies": {
"entities": "^2.0.0",
"fast-xml-parser": "^3.16.0",
"json2typescript": "^1.2.5",
"mxgraph": "4.1.0"
}
}
19 changes: 11 additions & 8 deletions src/component/parser/json/BpmnJsonParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,22 +13,25 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { Definitions } from './Definitions';
import BpmnModel from '../../../model/bpmn/BpmnModel';
import JsonConvertConfig from './converter/JsonConvertConfig';
import { JsonConvert } from 'json2typescript';
import { BpmnJsonModel } from '../xml/bpmn-json-model/BPMN20';
import { BpmnJsonModel, TDefinitions } from '../xml/bpmn-json-model/BPMN20';
import CollaborationConverter from './converter/CollaborationConverter';
import ProcessConverter from './converter/ProcessConverter';
import DiagramConverter from './converter/DiagramConverter';

export default class BpmnJsonParser {
constructor(readonly jsonConvert: JsonConvert) {}
constructor(readonly collaborationConverter: CollaborationConverter, readonly processConverter: ProcessConverter, readonly diagramConverter: DiagramConverter) {}

public parse(json: BpmnJsonModel): BpmnModel {
const definitions = this.jsonConvert.deserializeObject(json.definitions, Definitions);
return definitions.bpmnModel;
const definitions: TDefinitions = json.definitions;

this.collaborationConverter.deserialize(definitions.collaboration);
this.processConverter.deserialize(definitions.process);
return this.diagramConverter.deserialize(definitions.BPMNDiagram);
}
}

export function defaultBpmnJsonParser(): BpmnJsonParser {
// TODO replace the function by dependency injection, see #110
return new BpmnJsonParser(JsonConvertConfig.jsonConvert());
return new BpmnJsonParser(new CollaborationConverter(), new ProcessConverter(), new DiagramConverter());
}
51 changes: 0 additions & 51 deletions src/component/parser/json/Definitions.ts

This file was deleted.

15 changes: 1 addition & 14 deletions src/component/parser/json/converter/AbstractConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonConverter, JsonCustomConvert } from 'json2typescript';
import JsonConvertConfig from './JsonConvertConfig';

function convertEmptyStringAndObject<T>(element: string | T, acceptEmptyString: boolean): T {
if (element === '') {
return acceptEmptyString ? ({} as T) : undefined;
Expand All @@ -37,17 +34,7 @@ export function ensureIsArray<T>(elements: (T | string)[] | T | string, acceptEm
return returnedArray.filter(value => value);
}

@JsonConverter
export abstract class AbstractConverter<T> implements JsonCustomConvert<T> {
// TODO find a way to inject JsonConvert, see #110
protected readonly jsonConvert = JsonConvertConfig.jsonConvert();

// eslint-disable-next-line @typescript-eslint/no-unused-vars,@typescript-eslint/no-explicit-any
serialize(data: T): any {
// TODO throw exception
console.error('Not implemented !!');
}

export abstract class AbstractConverter<T> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
abstract deserialize(data: any): T;
}
9 changes: 2 additions & 7 deletions src/component/parser/json/converter/CollaborationConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonConverter } from 'json2typescript';
import { AbstractConverter, ensureIsArray } from './AbstractConverter';
import { Participant } from '../../../../model/bpmn/shape/ShapeBpmnElement';
import { Collaboration } from '../Definitions';
import { MessageFlow } from '../../../../model/bpmn/edge/Flow';
import { FlowKind } from '../../../../model/bpmn/edge/FlowKind';
import { TCollaboration } from '../../xml/bpmn-json-model/baseElement/rootElement/collaboration';
Expand All @@ -38,17 +36,14 @@ export function findMessageFlow(id: string): MessageFlow {
return convertedMessageFlows.find(i => i.id === id);
}

@JsonConverter
export default class CollaborationConverter extends AbstractConverter<Collaboration> {
deserialize(collaborations: string | TCollaboration | (string | TCollaboration)[]): Collaboration {
export default class CollaborationConverter extends AbstractConverter<void> {
deserialize(collaborations: string | TCollaboration | (string | TCollaboration)[]): void {
try {
// Deletes everything in the array, which does hit other references. For better performance.
convertedProcessRefParticipants.length = 0;
convertedMessageFlows.length = 0;

ensureIsArray(collaborations).forEach(collaboration => this.parseCollaboration(collaboration));

return {};
} catch (e) {
// TODO error management
console.error(e as Error);
Expand Down
10 changes: 3 additions & 7 deletions src/component/parser/json/converter/DiagramConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonConverter } from 'json2typescript';
import { AbstractConverter, ensureIsArray } from './AbstractConverter';
import Shape from '../../../../model/bpmn/shape/Shape';
import Bounds from '../../../../model/bpmn/Bounds';
Expand All @@ -37,7 +36,6 @@ function findProcessElement(participantId: string): ShapeBpmnElement {
}
}

@JsonConverter
export default class DiagramConverter extends AbstractConverter<BpmnModel> {
private convertedFonts: Map<string, Font> = new Map();

Expand Down Expand Up @@ -134,7 +132,7 @@ export default class DiagramConverter extends AbstractConverter<BpmnModel> {
private deserializeBounds(boundedElement: BPMNShape | BPMNLabel): Bounds {
const bounds = boundedElement.Bounds;
if (bounds) {
return this.jsonConvert.deserializeObject(bounds, Bounds);
return new Bounds(bounds.x, bounds.y, bounds.width, bounds.height);
}
}

Expand All @@ -151,10 +149,8 @@ export default class DiagramConverter extends AbstractConverter<BpmnModel> {
});
}

private deserializeWaypoints(waypoint: Point[]): Waypoint[] {
if (waypoint) {
return this.jsonConvert.deserializeArray(ensureIsArray(waypoint), Waypoint);
}
private deserializeWaypoints(waypoints: Point[]): Waypoint[] {
return ensureIsArray(waypoints).map(waypoint => new Waypoint(waypoint.x, waypoint.y));
Copy link
Member

Choose a reason for hiding this comment

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

👍 for the check removal

}

private deserializeLabel(bpmnLabel: string | BPMNLabel, id: string): Label {
Expand Down
42 changes: 0 additions & 42 deletions src/component/parser/json/converter/JsonConvertConfig.ts

This file was deleted.

9 changes: 2 additions & 7 deletions src/component/parser/json/converter/ProcessConverter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,9 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonConverter } from 'json2typescript';
import { AbstractConverter, ensureIsArray } from './AbstractConverter';
import ShapeBpmnElement, { ShapeBpmnBoundaryEvent, ShapeBpmnEvent, ShapeBpmnSubProcess } from '../../../../model/bpmn/shape/ShapeBpmnElement';
import { ShapeBpmnElementKind } from '../../../../model/bpmn/shape/ShapeBpmnElementKind';
import { Process } from '../Definitions';
import { AssociationFlow, SequenceFlow } from '../../../../model/bpmn/edge/Flow';
import { ShapeBpmnEventKind, supportedBpmnEventKinds } from '../../../../model/bpmn/shape/ShapeBpmnEventKind';
import ShapeUtil, { BpmnEventKind } from '../../../../model/bpmn/shape/ShapeUtil';
Expand Down Expand Up @@ -68,10 +66,9 @@ interface EventDefinition {
counter: number;
}

@JsonConverter
export default class ProcessConverter extends AbstractConverter<Process> {
export default class ProcessConverter extends AbstractConverter<void> {
Copy link
Member

Choose a reason for hiding this comment

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

❓ I thought that we would have been able to remove global 'converted' arrays and encapsulate them in a dedicated processing short life instance to avoid any side effect/concurrent accesses?
Is this plan in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think about that.
I don't really see how to do that.
If you want, do it. Or we can do it in another PR. As you wish ^^

Copy link
Member

Choose a reason for hiding this comment

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

so in another PR, later 😺

// eslint-disable-next-line @typescript-eslint/no-explicit-any
deserialize(processes: string | TProcess | (string | TProcess)[]): Process {
deserialize(processes: string | TProcess | (string | TProcess)[]): void {
try {
// Deletes everything in the array, which does hit other references. For better performance.
convertedFlowNodeBpmnElements.length = 0;
Expand All @@ -82,8 +79,6 @@ export default class ProcessConverter extends AbstractConverter<Process> {
defaultSequenceFlowIds.length = 0;

ensureIsArray(processes).forEach(process => this.parseProcess(process));

return {};
} catch (e) {
// TODO error management
console.error(e as Error);
Expand Down
35 changes: 1 addition & 34 deletions src/model/bpmn/Bounds.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,39 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonObject, JsonProperty } from 'json2typescript';

@JsonObject('Bounds')
export default class Bounds {
@JsonProperty('x', Number)
private _x: number;

@JsonProperty('y', Number)
private _y: number;

@JsonProperty('width', Number)
private _width: number;

@JsonProperty('height', Number)
private _height: number;

public constructor(x?: number, y?: number, width?: number, height?: number) {
this._x = x;
this._y = y;
this._width = width;
this._height = height;
}

get height(): number {
return this._height;
}
get width(): number {
return this._width;
}
get y(): number {
return this._y;
}
get x(): number {
return this._x;
}
public constructor(readonly x: number, readonly y: number, readonly width: number, readonly height: number) {}
}
21 changes: 1 addition & 20 deletions src/model/bpmn/edge/Waypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,6 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { JsonProperty } from 'json2typescript';

export default class Waypoint {
@JsonProperty('x', Number)
private readonly _x: number;

@JsonProperty('y', Number)
private readonly _y: number;

constructor(x?: number, y?: number) {
this._x = x;
this._y = y;
}

public get y(): number {
return this._y;
}

public get x(): number {
return this._x;
}
constructor(readonly x: number, readonly y: number) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ function executeEventCommonTests(
});

it(`should NOT convert, when there are several '${eventDefinitionKind}EventDefinition' in the same element${specificTitle}, ${titleForEventDefinitionIsAttributeOf}`, () => {
const eventDefinition = [{}, {}];
const eventDefinition = ['', {}];
Copy link
Member Author

Choose a reason for hiding this comment

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

Normally, it was in the previous PR, but I think there was a problem in the rebase 😕

const json = buildDefinitionsAndProcessWithTask();
addEvent(json, bpmnKind, { ...buildEventDefinitionParameter, eventDefinition }, specificBuildEventParameter);

Expand Down
4 changes: 2 additions & 2 deletions test/unit/component/parser/json/JsonBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { TFlowNode } from '../../../../../src/component/parser/xml/bpmn-json-mod
import { BPMNPlane, BPMNShape } from '../../../../../src/component/parser/xml/bpmn-json-model/BPMNDI';

type BPMNTEvent = TCatchEvent | TThrowEvent | TBoundaryEvent;
type BPMNEventDefinition = string | TEventDefinition | TEventDefinition[];
type BPMNEventDefinition = string | TEventDefinition | (string | TEventDefinition)[];

export enum EventDefinitionOn {
NONE,
Expand Down Expand Up @@ -140,7 +140,7 @@ export function addEventDefinitionsOnDefinition(jsonModel: BpmnJsonModel, buildP
const eventDefinition = buildParameter.eventDefinition ? buildParameter.eventDefinition : { id: 'event_definition_id' };
addEventDefinitions(jsonModel.definitions, { ...buildParameter, eventDefinition });
if (Array.isArray(eventDefinition)) {
event.eventDefinitionRef = eventDefinition.map(eventDefinition => eventDefinition.id);
event.eventDefinitionRef = eventDefinition.map(eventDefinition => (typeof eventDefinition === 'string' ? eventDefinition : eventDefinition.id));
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as previous comment

} else {
event.eventDefinitionRef = (eventDefinition as TEventDefinition).id;
}
Expand Down