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 definitions #646

Merged
merged 1 commit into from
Nov 24, 2017
Merged

Conversation

mxl
Copy link
Contributor

@mxl mxl commented Oct 5, 2017

@mxl mxl mentioned this pull request Oct 5, 2017
types/index.d.ts Outdated

export function withGoogleMap<P>(wrappedComponent: string | ComponentClass<P> | StatelessComponent<P>): ComponentClass<P & WithGoogleMapProps>

export interface GoogleMapProps {
Copy link
Owner

Choose a reason for hiding this comment

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

How do you generate these type definitions? By hand?

Could they be generate typed code from src/macro directly using jscodeshift API? That could save you lots of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see any documentation on how to do this. I could possibly spend time and write definitions generator that uses jscodeshift API but it will take much more time than writing these definitions. Also I'm sure that it will still require some manual adjustments.

@mxl
Copy link
Contributor Author

mxl commented Oct 6, 2017

@tomchentw You can review definitions. The only thing left is to move InfoBox definitions to https://github.com/lucasfs7/google-maps-infobox-module or https://github.com/DefinitelyTyped/DefinitelyTyped.

@tomchentw
Copy link
Owner

Just had a quick review, it looks awesome! Thanks for the hard work!

@mxl : The only thing left is to move InfoBox definitions to https://github.com/lucasfs7/google-maps-infobox-module or https://github.com/DefinitelyTyped/DefinitelyTyped.

They both looks fine to me. Would you raise up a PR there?

@mxl
Copy link
Contributor Author

mxl commented Nov 3, 2017

@tomchentw Thanks for the review! I'm back! lucasfs7/google-maps-infobox-module#8

@mxl mxl changed the title [WIP] Typescript definitions Typescript definitions Nov 13, 2017
@mxl
Copy link
Contributor Author

mxl commented Nov 13, 2017

@tomchentw It's ready for a final review and merging.

@moorea
Copy link

moorea commented Nov 21, 2017

@mxl, you are a lifesaver! If I could buy you a beer, I would 🍺

@NonMint
Copy link

NonMint commented Nov 23, 2017

Fantastic! I was just hoping for typescript definitions for this plugin. @mxl well done!

@tomchentw
Copy link
Owner

@mxl you're definitely awesome creating all those PRs with a clear roadmap! Great job!
I haven't tried this myself but I trust you.

@tomchentw tomchentw merged commit 072b4c7 into tomchentw:master Nov 24, 2017
@tomchentw
Copy link
Owner

Released v9.3.0

@mxl
Copy link
Contributor Author

mxl commented Nov 24, 2017

Guys, thank you for the kind words! @tomchentw if there will be any issue with definitions feel free to ping me.

@mxl mxl deleted the typescript-definitions branch November 24, 2017 10:41
attribution?: google.maps.Attribution
clickable?: boolean
cursor?: string
draggable?: string
Copy link

Choose a reason for hiding this comment

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

Should be boolean, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thanks! #710

@itsdouges
Copy link

itsdouges commented Nov 30, 2017

@mxl hey mate, getting these errors when running:

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(77,29): error TS2304: Cannot find name 'Calculator'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(87,25): error TS2304: Cannot find name 'ClusterIconStyle'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(93,22): error TS2304: Cannot find name 'Calculator'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(103,18): error TS2304: Cannot find name 'ClusterIconStyle'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(107,27): error TS2304: Cannot find name 'Cluster'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(110,24): error TS2304: Cannot find name 'Cluster'.

ERROR in /Users/madou/projects/a/node_modules/react-google-maps/types/index.d.ts
(111,25): error TS2304: Cannot find name 'Cluster'.

missing defs - oversight or something on my end?

@moorea
Copy link

moorea commented Nov 30, 2017

@Madou, sounds like you need to install the latest version of MarkerClustererPlus. Check out mxl’s first comments on this PR. He had to add some stuff there as well. Best of luck 👍🏻

@itsdouges
Copy link

@moorea right you are, thanks!

@mxl
Copy link
Contributor Author

mxl commented Dec 1, 2017

@moorea I included @types/markerclustererplus and @types/googlemaps in peerDependencies to avoid unnecessary dependencies for those who does not use TypeScript.

@phillipj
Copy link

@mxl mind elaborating the added value of making those peerDependencies? I assume those type definitions isn't required for using this wonderful package?

$ npm install

..

npm WARN react-google-maps@9.4.2 requires a peer of @types/googlemaps@^3.0.0 but none was installed.
npm WARN react-google-maps@9.4.2 requires a peer of @types/markerclustererplus@^2.1.29 but none was installed.

As we're not using Typescript in that project (or any other for that matter), installing Typescript definitions is not something we want. Most importantly, we don't want to ignore npm warnings either 🤔

@mxl
Copy link
Contributor Author

mxl commented Dec 17, 2017

@phillipj Actually without those dependencies one will get compilation errors as @Madou did so we can not just remove them from package.json. Maybe it will be simpler for everyone to move them to dependencies. This will remove warnings for those who use JavaScript and errors for those who use TypeScript. WDYT?

@phillipj
Copy link

@mxl yupp, at first glance that sounds like a better option to me 👍 Although we non-typescript-users would have to download a couple of superfluous packages, we won't have to explicitly install them as our own dependencies.

@kentaromiura
Copy link

kentaromiura commented Dec 20, 2017

Just having this issue and searching bring me here as npm list will show peer dependencies as UNMET PEER DEPENDENCY;

@mxl I think that if you remove the type definitions from peerDependencies and just leave them in devDependencies should work and make everyone happy :)

@mxl
Copy link
Contributor Author

mxl commented Dec 20, 2017

@kentaromiura This will remove UNMET PEER DEPENDENCY warnings but will not install those dependencies. So developers will have to manually navigate this package package.json and install missing definitions. I see only two ways to make package work "out of the box" for both JS and TS developers:

  1. move peerDependencies to dependencies;
  2. move definitions to separate package.

@tomchentw WDYT?

@tomchentw
Copy link
Owner

@mxl I don't have strong opinion on this. Is there any disadvantage using dependencies over peerDependencies? I can think one is outdated dependencies. Any thoughts?

@tomchentw
Copy link
Owner

Let's move the typings discussion to #737

@AbhaysinghBhosale
Copy link

Is this issues still tracked. Not sure if this library is still maintained

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.

Create TypeScript definitions
9 participants