search

parker-codes / coda-js

An eloquent Node API for interacting with your Coda Docs.
39 stars 9 forks source link

Unwanted console.error() when there is error during the request #11

Closed thang-tran-niteco closed 4 years ago

thang-tran-niteco commented 4 years ago

Hi,

When calling a request, e.g. getTable() and there is an error, the code catches the error and dumps its content to the console.log(). It creates problem as the stdout will be consumed downstream and the error populates the output stream even try catch was used.

I traced down the code was at https://github.com/parker-codes/coda-js/blob/master/src/API.ts

 async request(url: string, params: any = {}, method: Method = 'GET'): Promise<any> {
    try {
      const options: AxiosRequestConfig = ['POST', 'PUT', 'PATCH'].includes(method.toUpperCase()) ? { url, method, data: params } : { url, method, params };

      return await this._axiosInstance(options);
    } catch (error) {
      console.error(error);
    }
  }

Please consider to remove your try catch and let the caller handle it.

parker-codes commented 4 years ago

@mjoyce @svennu @inguelberth @fenixsolorzano As other users of this package, what do you think?

Does it make more sense to let users directly capture the HTTP error, or is someone up for creating custom Error types for the various Coda errors? That could potentially be time consuming, but the best option.

Either way, this would be a breaking change and require a new version. I don't think I care for a "new" version that seems to have less features (telling the dev to implement their own system).

@thang-tran-niteco I would appreciate your thoughts as well. If you'd be up for a PR on this, I can assist.

inguelberth commented 4 years ago

I agree with @thang-tran-niteco and I think creating custom Error types for the various coda errors is the best way, but also I think we can start removing that try-catch

parker-codes commented 4 years ago

I'm working on this - should hopefully finish it tonight.

thang-tran-niteco commented 4 years ago

Hi @parker-codes, I think there is a quick workaround by introducing an event handler with console.error() as default e.g. onRequestError: (error: Error) => console.error(error);

Then in the catch use onRequestError(error) instead.

So there will be no breaking change, and the user can customize the error if needed codajs.onRequestError = () => {}.

parker-codes commented 4 years ago

v2.4.0 released in https://github.com/parker-codes/coda-js/commit/bba7585eeeaf774ff7809cc6035bdc6db99d347e !

Includes BadRequestError, UnauthorizedError, ForbiddenError, NotFoundError, and TooManyRequestsError.

I decided it wasn't a breaking change for what was originally intended, so it's just a minor. I wasn't able to test each one for various reasons. Let me know if any of you have issues. We'll need to update the docs with error handling examples.

thang-tran-niteco commented 4 years ago

Hi @parker-codes,

After upgrading to v2.4.0 the code stops working.

import * as c from "coda-js";
import Coda from "coda-js";

d("c", c);
d("Coda", Coda);
const coda = new Coda(accountToken);
d("whoAmI", coda.whoAmI());

Its output is:

coda.insert c {
  default: {
    default: [Function: y],
    BadRequestError: [Function: e],
    UnauthorizedError: [Function: e],
    ForbiddenError: [Function: e],
    NotFoundError: [Function: e],
    TooManyRequestsError: [Function: e]
  },
  BadRequestError: [Function: e],
  UnauthorizedError: [Function: e],
  ForbiddenError: [Function: e],
  NotFoundError: [Function: e],
  TooManyRequestsError: [Function: e]
} +0ms
coda.insert Coda {
  default: [Function: y],
  BadRequestError: [Function: e],
  UnauthorizedError: [Function: e],
  ForbiddenError: [Function: e],
  NotFoundError: [Function: e],
  TooManyRequestsError: [Function: e]
} +0ms
ERROR TypeError: coda_js_1.default is not a constructor
    at getTable (/d/git/pet/nodejs/ptt/ptt-coda/commands.ts:74:22)
    at Command.insert (/d/git/pet/nodejs/ptt/ptt-coda/commands.ts:42:25)
    at Command.listener [as _actionHandler] (/d/git/pet/nodejs/ptt/ptt-coda/node_modules/.pnpm/commander@5.1.0/node_modules/commander/index.js:413:31)
    at Command._parseCommand (/d/git/pet/nodejs/ptt/ptt-coda/node_modules/.pnpm/commander@5.1.0/node_modules/commander/index.js:914:14)
    at Command._dispatchSubcommand (/d/git/pet/nodejs/ptt/ptt-coda/node_modules/.pnpm/commander@5.1.0/node_modules/commander/index.js:865:18)
    at Command._parseCommand (/d/git/pet/nodejs/ptt/ptt-coda/node_modules/.pnpm/commander@5.1.0/node_modules/commander/index.js:882:12)
    at Command.parse (/d/git/pet/nodejs/ptt/ptt-coda/node_modules/.pnpm/commander@5.1.0/node_modules/commander/index.js:717:10)
    at Object.<anonymous> (/d/git/pet/nodejs/ptt/ptt-coda/index.ts:14:15)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Module.m._compile (/home/user/.npm-global/pnpm-global/3/node_modules/.pnpm/registry.npmjs.org/ts-node/8.9.0_typescript@3.8.3/node_modules/ts-node/src/index.ts:839:23)
parker-codes commented 4 years ago

@thang-tran-niteco Ah, it looks like there's an issue with having a default export and named exports for the errors.. I'll see what I can do.

Edit: The most viable solution would be to make everything a named export. This will definitely be a breaking change.

import { Coda } from 'coda-js';

parker-codes commented 4 years ago

@thang-tran-niteco I just released v3.0.0 0c5876d5ad5ef67e66300a6665d6fb0f97c7effb - note that it is a breaking change as I said before.