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

Implement demo of using JSDoc to support type checking js #2502

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

Conversation

adamduren
Copy link
Contributor

@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-native repo. 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.json types | typings field and then the ionic-native plugin could list this as a devDependency. 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 includes field of tsconfig and that will provide type checking moving forward.

@@ -43,7 +43,8 @@
}
],
"scripts": {
"test": "jest && npm run eslint",
"check-types": "tsc",
"test": "npm run check-types && npm run jest && npm run eslint",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This will make sure we are passing the appropriate params to google maps api.

@@ -1,5 +1,6 @@


// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because of cordova custom module resolution

@@ -146,6 +147,12 @@ QUEUE.on('next', function() {
});

module.exports = {
/**
*
* @param {(result: GeocoderResult[]) => void} onSuccess
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This provides type checking by using JsDoc format.

"compilerOptions": {
"target": "es5",
"module": "commonjs",
"allowJs": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Add sources as we go to incrementally type the project over time.

@adamduren
Copy link
Contributor Author

Just for an example here is the output for npm run test catching a typo I made for demonstration purposes.

➜  cordova-plugin-googlemaps git:(discuss-embeding-type-info) ✗ npm run check-types

> [email protected] 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.

@adamduren
Copy link
Contributor Author

Type issues would also be caught by TravisCI when merging PRs adding another level of protection.

@wf9a5m75
Copy link
Member

Moving to TypeScript enforces me to tons of work.
Because of that, I can't do that at this time, maybe in next year.

@adamduren
Copy link
Contributor Author

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.

@wf9a5m75
Copy link
Member

It's time to migrate to TypeScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants