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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypeScript type definitions #66

Merged
merged 2 commits into from
Nov 14, 2018
Merged

Add TypeScript type definitions #66

merged 2 commits into from
Nov 14, 2018

Conversation

hassankhan
Copy link
Contributor

Closes #28.

Since this project uses "Lambda" types for some things, for example event and context, I added @types/aws-lambda as a devDependency. My reasoning being anyone using TypeScript with Lambda should/would already have that package installed. I suppose we could add it to peerDependencies, but that would be annoying for non-TS users.

I still have a bit of cleanup to do, any issues/feedback is most certainly welcome 馃憤

@coveralls
Copy link

coveralls commented Oct 22, 2018

Pull Request Test Coverage Report for Build 167

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.433%

Totals Coverage Status
Change from base Build 163: 0.0%
Covered Lines: 501
Relevant Lines: 503

馃挍 - Coveralls

@hassankhan hassankhan changed the title [WIP]: Add TypeScript type definitions [WIP] Add TypeScript type definitions Oct 23, 2018
@hassankhan hassankhan changed the title [WIP] Add TypeScript type definitions Add TypeScript type definitions Oct 23, 2018
@hassankhan
Copy link
Contributor Author

Finished adding as much as I could glean from the docs 馃憤

@hassankhan hassankhan changed the title Add TypeScript type definitions [WIP] Add TypeScript type definitions Oct 24, 2018
@hassankhan hassankhan changed the title [WIP] Add TypeScript type definitions Add TypeScript type definitions Oct 24, 2018
@hassankhan
Copy link
Contributor Author

hassankhan commented Oct 24, 2018

Hi @jeremydaly, PR is ready for review 馃槃

@jeremydaly
Copy link
Owner

Awesome work, @hassankhan! I will give this a review in the next few days. Thanks for the PR!

@jeremydaly jeremydaly self-assigned this Oct 24, 2018
@jeremydaly jeremydaly added this to the v0.9 milestone Oct 24, 2018
@hassankhan
Copy link
Contributor Author

Hi @jeremydaly, any chance you could review the PR soon? 馃槃

@jeremydaly
Copy link
Owner

I'm trying to get to it. It's been a busy week!

@jeremydaly jeremydaly merged commit 262f377 into jeremydaly:master Nov 14, 2018
@Wintereise
Copy link
Contributor

Wintereise commented Nov 20, 2018

Hi, so, this might be a newbie question, but how do you use this?

Attempting to import any of these defined types throws an error, like so:

ERROR in G:\xen\manager\src\core\app\App.ts
./src/core/app/App.ts
[tsl] ERROR in G:\xen\manager\src\core\app\App.ts(3,10)
      TS2305: Module '"G:/xen/manager/node_modules/lambda-api/index"' has no exported member 'API'

Am I doing something wrong? Or it seems like the definitions haven't been marked as exported?

Looking at type definitions for similar projects, they all seem to mark their objects as exported -
Auth0: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/auth0/index.d.ts
AWS Lambda: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/aws-lambda/index.d.ts

@hassankhan

@Wintereise
Copy link
Contributor

I added export to the classes + fixed the middleware signatures (use takes a callback for both errors and normal instances, not individual request/response/next instances).

I put that version of it here: https://github.com/Wintereise/lambda-api/commit/6acd197540d940445eb02e5afcbafad41f80bcd9

Can you guys review? If all good, I'll open another pull request and add the new Error types in 0.9.

@hassankhan @jeremydaly

@hassankhan
Copy link
Contributor Author

hassankhan commented Nov 20, 2018

Good catch @Wintereise, thanks for adding the exports. I've added a comment to your branch, aside from that all looks good to me 馃憤

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.

None yet

4 participants