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

TypeScript regression #63

Closed
whaatt opened this issue Sep 5, 2019 · 5 comments · Fixed by #65
Closed

TypeScript regression #63

whaatt opened this issue Sep 5, 2019 · 5 comments · Fixed by #65

Comments

@whaatt
Copy link

whaatt commented Sep 5, 2019

#55 added auto-generated TypeScript types, but it seems like some of the functions (e.g. h3ToGeoBoundary) that return an array of arrays (i.e. Array[]) don't type-check properly in TypeScript, since the compiler expects the outer Array to have a type parameter:

* @return {Array[]} Array of [lat, lng] pairs

The fix is likely to specify the inner-most type explicitly (e.g. Number[][]) in the JSDoc.

@nrabinowitz
Copy link
Collaborator

Thanks for the bug report. We probably need to add a check in Travis CI to catch this kind of issue.

@whaatt
Copy link
Author

whaatt commented Sep 12, 2019

Will this be addressed in any upcoming releases? Would love to have type enforcement when using this library in TypeScript (I'm stuck on 3.4.0 at the moment).

Thanks!

@nrabinowitz
Copy link
Collaborator

Yes, I'd like to fix it in the next release. I'm not as familiar with TS as I ought to be - is there a standard type-checking step we could do in CI that would catch this?

@whaatt
Copy link
Author

whaatt commented Sep 12, 2019

Nothing off-the-shelf AFAIK, but it's simple enough to add a script in package.json, e.g.:

"scripts": {
  // ...
  "type-check": "tsc dist/types.d.ts"
  // ...
}

I'm guessing you do an npm install already in your CI, so you could add typescript as a dev dependency to get tsc in your local environment. If I run this command now, I get the following error output:

WorkMac:h3-js sanj$ tsc dist/types.d.ts
dist/types.d.ts:80:75 - error TS2314: Generic type 'Array<T>' requires 1 type argument(s).

80     function h3ToGeoBoundary(h3Index: H3Index, formatAsGeoJson: boolean): Array[];
                                                                             ~~~~~

dist/types.d.ts:137:36 - error TS2314: Generic type 'Array<T>' requires 1 type argument(s).

137     function polyfill(coordinates: Array[], res: number, isGeoJson: boolean): H3Index[];
                                       ~~~~~

dist/types.d.ts:149:83 - error TS2314: Generic type 'Array<T>' requires 1 type argument(s).

149     function h3SetToMultiPolygon(h3Indexes: H3Index[], formatAsGeoJson: boolean): Array[];
                                                                                      ~~~~~

dist/types.d.ts:228:93 - error TS2314: Generic type 'Array<T>' requires 1 type argument(s).

228     function getH3UnidirectionalEdgeBoundary(edgeIndex: H3Index, formatAsGeoJson: boolean): Array[];
                                                                                                ~~~~~


Found 4 errors.

@nrabinowitz
Copy link
Collaborator

Thanks, this is perfect.

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 a pull request may close this issue.

2 participants