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.

Sign up for GitHub

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

Jump to bottom

[REFACTOR] Remove json2typescript dependency #443

Merged
merged 7 commits into from
Aug 4, 2020
Merged

[REFACTOR] Remove json2typescript dependency #443

merged 7 commits into from
Aug 4, 2020

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Jul 30, 2020 •
edited by tbouffard
Loading

  • Remove json2typescript dependency
  • Refactor the converter

Closes #383

@csouchet csouchet added dependencies Pull requests that update a dependency (dev or runtime) depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first refactoring Code refactoring labels Jul 30, 2020
@csouchet csouchet force-pushed the Refactor/Remove_json2typescript branch from 6a41645 to 0886bc8 Compare July 30, 2020 13:11
@csouchet csouchet mentioned this pull request Jul 30, 2020
Merged
@csouchet csouchet force-pushed the Refactor/Remove_json2typescript branch 2 times, most recently from 1a0c839 to 6136bfb Compare August 3, 2020 10:16
@csouchet csouchet marked this pull request as ready for review August 3, 2020 10:19
tbouffard
tbouffard reviewed Aug 3, 2020
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

⚠️ the documentation should be updated as we currently still reference json2typescript in the architecture section

@csouchet csouchet force-pushed the Refactor/Remove_json2typescript branch from 6136bfb to aa28a3e Compare August 3, 2020 15:12
@csouchet csouchet removed the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Aug 3, 2020
@csouchet
Copy link
Member Author

csouchet commented Aug 3, 2020

⚠️ the documentation should be updated as we currently still reference json2typescript in the architecture section

-> Documentation updated

@csouchet csouchet requested a review from tbouffard August 3, 2020 15:21
@csouchet csouchet force-pushed the Refactor/Remove_json2typescript branch from 6274cba to c552e36 Compare August 3, 2020 15:22
@tbouffard tbouffard mentioned this pull request Aug 3, 2020
Closed
@tbouffard
Copy link
Member

Notice that removing json2typescript decreases the size of our demo js file: from 143.9 kb (v0.1.6) to 126.66 kb (including our json-bpmn model)

csouchet
csouchet commented Aug 3, 2020
View reviewed changes
test/unit/component/parser/json/BpmnJsonParser.event.test.ts
@@ -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 😕

csouchet
csouchet commented Aug 3, 2020
View reviewed changes
test/unit/component/parser/json/JsonBuilder.ts
@@ -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

tbouffard
tbouffard reviewed Aug 3, 2020
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

Some questions, otherwise looks good

src/component/parser/json/converter/DiagramConverter.ts
if (waypoints) {
return ensureIsArray(waypoints).map(waypoint => new Waypoint(waypoint.x, waypoint.y));
}
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

src/component/parser/json/converter/AbstractConverter.ts Outdated Show resolved Hide resolved
src/component/parser/json/converter/ProcessConverter.ts
@@ -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 😺

@csouchet csouchet merged commit 1e1a566 into master Aug 4, 2020
@csouchet csouchet deleted the Refactor/Remove_json2typescript branch August 4, 2020 07:32
@tbouffard tbouffard mentioned this pull request Aug 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbouffard tbouffard tbouffard approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency (dev or runtime) refactoring Code refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[INFRA] investigate appearance of Microsoft Copyright License
2 participants
@csouchet @tbouffard