Implement demo of using JSDoc to support type checking js #2502
Conversation
| @@ -43,7 +43,8 @@ | |||
| } | |||
| ], | |||
| "scripts": { | |||
| "test": "jest && npm run eslint", | |||
| "check-types": "tsc", | |||
| "test": "npm run check-types && npm run jest && npm run eslint", | |||
There was a problem hiding this comment.
Add type checking as part of the test process.
| @@ -55,8 +56,10 @@ | |||
| }, | |||
| "homepage": "https://github.com/mapsplugin/cordova-plugin-googlemaps", | |||
| "devDependencies": { | |||
| "@types/googlemaps": "^3.30.16", | |||
There was a problem hiding this comment.
This will make sure we are passing the appropriate params to google maps api.
| @@ -1,5 +1,6 @@ | |||
|
|
|||
|
|
|||
| // @ts-ignore | |||
There was a problem hiding this comment.
This is needed because of cordova custom module resolution
| @@ -146,6 +147,12 @@ QUEUE.on('next', function() { | |||
| }); | |||
|
|
|||
| module.exports = { | |||
| /** | |||
| * | |||
| * @param {(result: GeocoderResult[]) => void} onSuccess | |||
There was a problem hiding this comment.
This provides type checking by using JsDoc format.
| "compilerOptions": { | ||
| "target": "es5", | ||
| "module": "commonjs", | ||
| "allowJs": true, |
There was a problem hiding this comment.
allowJs and checkJs are what enable type checking. noEmit is added because we don't actually want to compile the files.
| "esModuleInterop": true | ||
| }, | ||
| "include": [ | ||
| "src/browser/PluginGeocoder.js", |
There was a problem hiding this comment.
Add sources as we go to incrementally type the project over time.
|
Just for an example here is the output for ➜ cordova-plugin-googlemaps git:(discuss-embeding-type-info) ✗ npm run check-types
> cordova-plugin-googlemaps@2.5.0 check-types /Users/adamduren/Workspace/cordova-plugin-googlemaps
> tsc
src/browser/PluginGeocoder.js:160:18 - error TS2551: Property 'positon' does not exist on type 'GeocoderRequest'. Did you mean 'position'?
160 if (!request.positon && request.address) {
~~~~~~~
typings/index.d.ts:45:3
45 position?: ILatLng | ILatLng[];
~~~~~~~~
'position' is declared here. |
|
Type issues would also be caught by TravisCI when merging PRs adding another level of protection. |
|
Moving to TypeScript enforces me to tons of work. |
|
Thanks for taking a look, I'll keep the PR open and update the branch as I have more time. One good thing about the approach is that it's a change that can happen incrementally vs all or nothing. |
|
It's time to migrate to TypeScript. |
@wf9a5m75 I am curious to get your thoughts on adding the type checking to the project without having to convert the entire project to typescript. Would this be something that you are in favor of?
Currently there are types in the
ionic-nativerepo. It seems like it would be beneficial to be able to reference them in this project as well to help keep the types in sync as well as providing type checking to this project which will help reduce chance of errors.This PR is to demo how it could be achieved. We could export the types from this project using the
package.jsontypes | typingsfield and then theionic-nativeplugin could list this as adevDependency. Another option would be to put the types in their own repo in which both this repo and the ionic-native one would depend on but I am leaning towards moving them into this project in order to reduce complexity.It would be an effort I can help with over time but as we move types over we can list files in the
includesfield oftsconfigand that will provide type checking moving forward.