-
Notifications
You must be signed in to change notification settings - Fork 17
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 support #53
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #53 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 51 53 +2
Branches 3 3
=====================================
+ Hits 51 53 +2
Continue to review full report at Codecov.
|
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 very much for this PR @voodoods
Looks very good to me. I'll take a better look at it later this week.
package.json
Outdated
@@ -2,19 +2,20 @@ | |||
"name": "aws-polly-voices", | |||
"version": "0.0.0-development", | |||
"description": "Helper module to get AWS polly voices in an idiomatic way", | |||
"main": "dist/index.js", | |||
"main": "dist/index.ts", |
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.
I don't think we should serve the ts file because a simple nodejs app doesn't support it out of the box. Instead, we should add the types
property.
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.
Thanks for pointing this out! This change was unintended, I reverted this. The build process has of course not been touched.
I've also added a types
property pointing to a new custom index.d.ts
type declaration file. However I have to point a few things out here:
- this is my first attempt wrting a custom file declaration file for a module; I had to do some research first to get it right - so please feel free to double check
- babel's build process won't do any type checking; I've added a new
check-types
command to use the Typescript compiler manually without emmiting output - I suggest to create a new issue to automatically apply type-checking to a commit-hook
Other than that: Sorry for the long delay 🙏
cbac0e7
to
406e5c7
Compare
update dependencies to work with typescript add typescript support to babel remove unnecessary ts compiler options
3ccbdde
to
62d813b
Compare
add welsh method type definiton fix class instance return type for methods
62d813b
to
0d887cf
Compare
@@ -3,18 +3,20 @@ | |||
"version": "0.0.0-development", | |||
"description": "Helper module to get AWS polly voices in an idiomatic way", | |||
"main": "dist/index.js", | |||
"types": "src/index.d.ts", |
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.
We should use a file generated inside the dist
folder, in order to always use the latest version of the types.
@@ -61,7 +69,8 @@ | |||
"jest-extended" | |||
], | |||
"collectCoverageFrom": [ | |||
"src/*.js" | |||
"src/*.js", | |||
"src/*.ts" |
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.
We should collect only coverage from the ts
files from now on.
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.
The index.ts
file doesn't have any type declaration at all, we need to add it to properties and methods. The file index.d.ts
should be generated based on the definitions of the main class.
Proposed changes
Added support for Typescript as requested in #44
In detail:
tsconfig.json
babel.config.js
.js
files with.ts
in./src
and./test
scripts
inpackage.json
and addedcheck-types
scriptREADME.md
Types of changes
What types of changes does your code introduce to Appium?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.