-
Notifications
You must be signed in to change notification settings - Fork 935
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
Conversation
types/index.d.ts
Outdated
|
||
export function withGoogleMap<P>(wrappedComponent: string | ComponentClass<P> | StatelessComponent<P>): ComponentClass<P & WithGoogleMapProps> | ||
|
||
export interface GoogleMapProps { |
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.
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.
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 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.
1b51ecc
to
acc309c
Compare
@tomchentw You can review definitions. The only thing left is to move |
Just had a quick review, it looks awesome! Thanks for the hard work!
They both looks fine to me. Would you raise up a PR there? |
@tomchentw Thanks for the review! I'm back! lucasfs7/google-maps-infobox-module#8 |
acc309c
to
552d7e3
Compare
@tomchentw It's ready for a final review and merging. |
@mxl, you are a lifesaver! If I could buy you a beer, I would 🍺 |
Fantastic! I was just hoping for typescript definitions for this plugin. @mxl well done! |
@mxl you're definitely awesome creating all those PRs with a clear roadmap! Great job! |
Released v9.3.0 |
Guys, thank you for the kind words! @tomchentw if there will be any issue with definitions feel free to ping me. |
attribution?: google.maps.Attribution | ||
clickable?: boolean | ||
cursor?: string | ||
draggable?: string |
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.
Should be boolean
, no?
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.
Yep, thanks! #710
@mxl hey mate, getting these errors when running:
missing defs - oversight or something on my end? |
@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 👍🏻 |
@moorea right you are, thanks! |
@moorea I included |
@mxl mind elaborating the added value of making those $ 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 🤔 |
@phillipj Actually without those dependencies one will get compilation errors as @Madou did so we can not just remove them from |
@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. |
Just having this issue and searching bring me here as npm list will show peer dependencies as @mxl I think that if you remove the type definitions from peerDependencies and just leave them in devDependencies should work and make everyone happy :) |
@kentaromiura This will remove
@tomchentw WDYT? |
@mxl I don't have strong opinion on this. Is there any disadvantage using |
Let's move the typings discussion to #737 |
Is this issues still tracked. Not sure if this library is still maintained |
Fixes #363
Roadmap: