Skip to content

Fix /auth/get response types - #162

Merged
ntindall merged 6 commits into
plaid:masterfrom
chris--young:types
Aug 3, 2018
Merged

Fix /auth/get response types#162
ntindall merged 6 commits into
plaid:masterfrom
chris--young:types

Conversation

@chris--young

Copy link
Copy Markdown
Contributor

Added a new response type for getAuth() since it should not use the same response type as getAccounts()

plaid.com/docs/api/#auth

> tsc

src/account/controller.ts:38:25 - error TS2339: Property 'numbers' does not exist on type 'AccountsResponse'.

38         console.log(res.numbers);
                           ~~~~~~~

Also moved types out of dev dependencies so that theyll be included with npm install --save plaid to prevent missing types

> tsc

node_modules/plaid/index.d.ts:2:31 - error TS7016: Could not find a declaration file for module 'request'. 'C:/Users/young/Desktop/book/node_modules/request/index.js' implicitly has an 'any' type.
  Try `npm install @types/request` if it exists or add a new declaration (.d.ts) file containing `declare module 'request';`

2   import { CoreOptions } from 'request';
                                ~~~~~~~~~

@chris--young

Copy link
Copy Markdown
Contributor Author

Fixes: #160

@diit

diit commented Jul 18, 2018

Copy link
Copy Markdown

We're having this issue as well, anything I can do to help get it merged in?

@ntindall ntindall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@chris--young left a few comments - great catch and thank you for working on this.

Comment thread index.d.ts Outdated

interface BaseResponse {
request_id: string;
status_code: number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't believe status_code is returned in the base response

Comment thread index.d.ts Outdated
account: string,
account_id: string,
routing: string,
write_routing: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

from the documentation for Auth see here this should be wire_routing

Comment thread index.d.ts Outdated
}

interface ACHNumbers {
account: string,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we use ; as the trailing delimiter on the typings (as we do elsewhere in the file)?

Comment thread package.json
"bluebird": "^3.5.1",
"ramda": "0.23.x",
"request": "^2.74.0"
"request": "^2.74.0",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍I believe this is the correct thing for us to do, given that we are exporting the typings from the package (for typescript clients, it will matter for those that do not install devDependencies).

@ntindall

ntindall commented Aug 3, 2018

Copy link
Copy Markdown
Contributor

@diit, we will merge it in as soon as the comments are addressed. Sorry for the delay!
cc @michaelckelly @nathankot @pbernasconi

@chris--young

Copy link
Copy Markdown
Contributor Author

no worries about the delay, just made a few changes to address these comments

also been having a hard time running getting this make file to work on windows might be willing to open up and issue and fix that if you think its worth it

@ntindall ntindall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread .gitignore
/node_modules/
/npm-debug.log
.env
.vs/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you probably need to add this to your .gitignore_global file @chris--young

I don't mind it being comitted here as well, though.

@ntindall
ntindall merged commit b0856da into plaid:master Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants