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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typescript type definitions #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add typescript type definitions #7

wants to merge 1 commit into from

Conversation

clems4ever
Copy link

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.

@clems4ever
Copy link
Author

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).
I will create a pull request soon for u2f-api.

@clems4ever
Copy link
Author

Hello @ashtuchkin , did you have time to check the PR?

@zcei
Copy link

zcei commented Sep 29, 2017

Out of interest: how does the testing of the typings work? You just have included the .ts file in include section of tsconfig and when it compiles it's correct?

Copy link
Owner

@ashtuchkin ashtuchkin left a 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;
Copy link
Owner

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.

Copy link
Author

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;
Copy link
Owner

Choose a reason for hiding this comment

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

nit: successful: true;

Copy link
Author

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;
}


Copy link
Owner

Choose a reason for hiding this comment

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

nit: double empty line.

Copy link
Author

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",
Copy link
Owner

Choose a reason for hiding this comment

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

thank you!

Copy link
Author

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 ."
},
Copy link
Owner

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.

Copy link
Author

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 @@

Copy link
Owner

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.

Copy link
Author

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 @@
{
Copy link
Owner

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.

Copy link
Author

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.

@ashtuchkin
Copy link
Owner

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`
@clems4ever
Copy link
Author

Everything should be fixed now. Sorry for the long delay as well :).

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