bpmn-io / bpmn-moddle

Read and write BPMN 2.0 XML from JavaScript.
MIT License
444 stars 162 forks source link

Promisify apis #73

Closed oguzeroglu closed 4 years ago

oguzeroglu commented 4 years ago

Closes #71

oguzeroglu commented 4 years ago

Please get rid of done entirely. This means that patterns such as this:

fromFile('test/fixtures/bpmn/error/no-xml.txt').catch(function(err) {
   expect(err).to.exist;
     done();
   });
});

...should be transformed to try { await ... } ... blocks.

In this test we benefit from the done() behavior, as we'd like to check if fromFile fails. The alternative would be:

var err;

try {
  // await stuff...
} catch(error){
  err = error;
}

expect(err).not.to.be.undefined;

Which in my opinion is not prettier, nor worth the effort just for the sake of getting rid of "done".

nikku commented 4 years ago

This does not only have the benefit of getting rid of done but also the benefit of being more understandable.

oguzeroglu commented 4 years ago

Updated the tests 👍

nikku commented 4 years ago

Added three changes on top:

Both follow the pattern we're taking with moddle-xml.

nikku commented 4 years ago

Looking at the API I find it a bit odd that read returns a structured object (including warnings) while write returns the plain result (i.e. only the written XML string).

I remember we had an issue somewhere that write warnings are currently simply emitted to the console, as the current API does not support exposing it. Returning a complex object ({ xml, warnings, ... }) would allow us to export that meta-data in the future. It would also mean that we offer the same, semantic API for read and write.

Getting the XML would still be as simple as

const { xml } = await moddle.write(definitions);

What do you think?

oguzeroglu commented 4 years ago

I updated the API accordingly, also squashed your commits.