twilio / twilio-node

Node.js helper library
MIT License
1.4k stars 510 forks source link

Incorrect Typescript types on MessageInstance #606

Open eugene-kim opened 4 years ago

eugene-kim commented 4 years ago

Issue Summary

The MessageInstance class looks as if it contains incorrect types for some of its instance properties. Properties such as errorCode and errorMessage can be nullable according to the documentation so I would think that the type of errorCode should be number | null rather than simply number as it currently is.

image

Props types that seem incorrect:

Code Snippet

Compare the type of the instance property errorCode here

declare class MessageInstance extends SerializableClass {
  /**
   * Initialize the MessageContext
   *
   * @param version - Version of the resource
   * @param payload - The instance payload
   * @param accountSid - The SID of the Account that created the resource
   * @param sid - The unique string that identifies the resource
   */
  constructor(version: V2010, payload: MessagePayload, accountSid: string, sid: string);

  private _proxy: MessageContext;
  accountSid: string;
  apiVersion: string;
  body: string;
  dateCreated: Date;
  dateSent: Date;
  dateUpdated: Date;
  direction: MessageDirection;
  errorCode: number;
  errorMessage: string;
  ...
}

to the sample JSON response in the documentation:

{
  "account_sid": "ACXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
  "api_version": "2010-04-01",
  "body": "body",
  "date_created": "Thu, 30 Jul 2015 20:12:31 +0000",
  "date_sent": "Thu, 30 Jul 2015 20:12:33 +0000",
  "date_updated": "Thu, 30 Jul 2015 20:12:33 +0000",
  "direction": "outbound-api",
  "error_code": null,
  "error_message": null,
  "from": "+15017122661",
  "messaging_service_sid": "MGXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
  "num_media": "0",
  "num_segments": "1",
  "price": null,
  "price_unit": null,
  "sid": "SMXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
  "status": "sent",
  "subresource_uris": {
    "media": "/2010-04-01/Accounts/ACXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/Messages/SMXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/Media.json"
  },
  "to": "+15558675310",
  "uri": "/2010-04-01/Accounts/ACXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/Messages/SMXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX.json"
}

Exception/Log

# paste exception/log here

Technical details:

eshanholtz commented 4 years ago

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog.

eugene-kim commented 4 years ago

@eshanholtz is there a source of truth one can use as a reference for the Message Resource schema? It would help anyone looking to create a PR or have a temporary work around for the type discrepancies.

childish-sambino commented 4 years ago

The source of truth is internal (and only has error_code marked as nullable). I've submitted an internal ticket to get the others updated as nullable (ref MSG-6006), but changes are also needed in the code generator to handle null-ables properly.

sambecker commented 4 years ago

UPDATE: fixed this by deleting my package-lock.json file ... looks like it was a DefinitelyTyped @types/express issue ... — Seeing similar TypeScript compilation errors when deploying firebase functions:

Type 'P' does not satisfy the constraint 'Params'.
Type 'P' is not assignable to type 'ParamsArray'.

relating to

extends core.ErrorRequestHandler<P, ResBody, ReqBody, ReqQuery> { }
interface Request<P = core.ParamsDictionary, ResBody = any, ReqBody = any, ReqQuery = core.Query> extends core.Request<P, ResBody, ReqBody, ReqQuery> { }
interface RequestHandler<P = core.ParamsDictionary, ResBody = any, ReqBody = any, ReqQuery = core.Query> extends core.RequestHandler<P, ResBody, ReqBody, ReqQuery> { }

Downgrading from 3.49.1 to 3.49.0 does not seem to help ...

Deploying functions to a Node.js 10 environment ...

Probably worth noting that firebase still uses tslint, not eslint for Typescript projects

sbdchd commented 3 years ago

Couple more mistyped fields on MessageInstance:

shellscape commented 3 years ago

10 months open now folks. Additionally, MessageInstance is all over this codebase, but there's no direct export in the Twilio lib, so importing the type is problematic and requires a path import, which is fragile and an antipattern.

tsongas commented 2 years ago

+1

davingee commented 1 year ago

+1

shekspir55 commented 1 year ago

not fixed yet