-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add typescript type definitions #7
base: master
Are you sure you want to change the base?
Conversation
For you information, I've also written the type definitions for the u2f-api npm package that I use in combination with u2f package in my project (https://www.npmjs.com/package/authelia). |
Hello @ashtuchkin , did you have time to check the PR? |
Out of interest: how does the testing of the typings work? You just have included the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, thank you for your contribution! Added a couple comments inline.
index.d.ts
Outdated
successful: boolean; | ||
publicKey: string; | ||
keyHandle: string; | ||
certificate: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: certificate is a Buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
index.d.ts
Outdated
} | ||
|
||
export interface RegistrationResult { | ||
successful: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: successful: true;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
index.d.ts
Outdated
certificate: string; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: double empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
package.json
Outdated
"homepage": "https://github.com/ashtuchkin/u2f", | ||
"devDependencies": { | ||
"mocha": "2" | ||
"@types/node": "^7.0.22", | ||
"mocha": "2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed @types/node and added ts-node instead.
package.json
Outdated
@@ -16,8 +17,13 @@ | |||
"bugs": { | |||
"url": "https://github.com/ashtuchkin/u2f/issues" | |||
}, | |||
"scripts": { | |||
"build": "./node_modules/.bin/tsc -p ." | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you remove this clause? it's not actually a build step, so might be confusing to others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,98 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing tests! I really appreciate it.
This file is not using mocha, though, so it can be confusing. Can you try to make it a real test? This article can help: https://journal.artfuldev.com/unit-testing-node-applications-with-typescript-using-mocha-and-chai-384ef05f32b2
Or, if you want to just keep it as an example of how typescript would work, then can you rename this file to typescript-example.ts
, so it doesn't look like a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. One can run the tests with mocha -r ts-node/register test/ts-test.ts
.
tsconfig.json
Outdated
@@ -0,0 +1,21 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project is not built in typescript, so this file could be confusing, could you remove it if possible? I think we can use node-ts
module to avoid compilation step altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File removed in favor of node-ts.
Sorry for such a long delay! |
Also add some unit tests that can be run with: `mocha -r ts-node/register test/ts-test.ts`
Everything should be fixed now. Sorry for the long delay as well :). |
Add type definitions and a test file in typescript to test the compilation.
npm run build
compiles the typescript test file in directory build/ so that you can run it and check everything is working fine.