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

Ensure Embark uses Error instances for error handling #911

Open
0x-r4bbit opened this issue Sep 26, 2018 · 1 comment
Open

Ensure Embark uses Error instances for error handling #911

0x-r4bbit opened this issue Sep 26, 2018 · 1 comment

Comments

@0x-r4bbit
Copy link
Contributor

Overview

TL;DR
Embark happens have different approaches to how and when errors are being thrown and handled. Many APIs are callback based at the moment and expect the first parameter of a given callback to be the error. E.g.

EmbarkJS.someAPI((err, data) => {
  if (err) {
     // whoops
  }
})

^ This is a common pattern in NodeJS land and callback based APIs.

As of right now, Embark doesn't actually emit instances of Error in such cases, but rather just strings. E.g.

EmbarkJS.someAPI = (callback) => {
  const err = 'this is an error message'
  callback(err)
};

This is not too much of a big deal, as we don't actually throw anything anyways. However, it'd be better if all APIs that emit some sort of error, actually emit Error instances so one can react accordingly, especially when different error types can be thrown.

E.g.

EmbarkJS.someAPI((err, data) => {
  if (err) {
    switch (err.type) {
       case EMBARK_ERR_CONN: 
         // ...
         break;
       ...
    }
  }
})

This works the same way with promise-based APIs, with and without async/await:

EmbarkJS.someAPI = (callback) => {
  return new Promise((resolve, reject) => {
    const err = 'some error message';
    reject(new Error(err))
  });
}

// and then

try {
  const someData = await EmbarkJS.someAPI()
} catch (e) {
   switch (e.type) { // or instanceof or whatever we decide to do
     ...
   }
}

We've discussed this here. This issue ensures we don't forget about it :)

@0x-r4bbit
Copy link
Contributor Author

We would then avoid having conditions like: https://github.com/embark-framework/embark/blob/b8d3f3cd11700b8da457b77a400760bfd6052c84/lib/modules/deployment/contract_deployer.js#L278

Which is very error prone given that we're relying on concrete partial strings here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Embark
  
Inbox
Development

No branches or pull requests

2 participants