Added definitions. #152
Added definitions. #152
Conversation
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
======================================
Coverage 89.1% 89.1%
======================================
Files 4 4
Lines 101 101
Branches 25 25
======================================
Hits 90 90
Misses 9 9
Partials 2 2Continue to review full report at Codecov.
|
react-sizeme.d.ts
Outdated
| noPlaceholder?: boolean; | ||
| } | ||
|
|
||
| export class SizeMe extends React.Component<SizeMeOptions> { |
There was a problem hiding this comment.
I think we should revise this typing to take into account it's render props based API.
I am not familiar with Typescript but found the following possibly helpful article:
https://medium.com/@jrwebdev/react-render-props-in-typescript-b561b00bc67c
There was a problem hiding this comment.
Thank you! I'll read the article.
There was a problem hiding this comment.
I believe from reading the article that this typing was missing on the SizeMeOptions interface:
children(props: SizeMeProps): JSX.Element;
Now TS complains if we pass anything but a function as a children of the <SizeMe> component, and we get the typings of SizeMeProps on that function as well. (Before on my code I had to manually specify {size} was SizeMeProps like this <SizeMe>{({size}: SizeMeProps) => ... </SizeMe>.
Also, I believe SizeMeProps interface doesn't have to be exported anymore.
There was a problem hiding this comment.
Ok, nice one, sounds good. Did you commit/push your latest? Not seeing these changes. 😀
There was a problem hiding this comment.
Great! I'll commit that change now that you agreed with it.
|
Thanks for picking this up @slig! Much appreciated. At first glance it looks great to myself. I think we just need to revise the render props API as commented. |
|
Great work. Will it land any time soon? |
react-sizeme.d.ts
Outdated
| @@ -0,0 +1,27 @@ | |||
| declare module 'react-sizeme' { | |||
There was a problem hiding this comment.
Actually, you don't need this, since your package.json includes types property.
I doubt that this will affect end-users somehow, but declare module syntax skipped in both DefinetlyTyped typings and generated by TypeScript compiler.
declare module used only for augmenting host by plugin (e.g. some packages for express do use declare module 'express' for best developers experience) or for typing untyped package from your actual code, when there are no typings for it neither in package nor @types/
There was a problem hiding this comment.
Thanks for reviewing, I'll remove that. I went to see some typings generated by the TS compiler and it emits export declare class Foo instead of just export class Foo. Should I add the declare as well? Thanks!
There was a problem hiding this comment.
I don't think so, I don't know why actually TS adds declare to declarations, but it doesn't seem to affect compilation.
But this affects readability of definitions (at least in VS Code), because breaks colorizing(I guess something is wrong with grammar when there are too many reserved words in one statement).
Also, I've just realized, that you don't have react import and you should.
I suggest to use import { Component, ComponentType } from 'react', so typings will work with both esModuleInterop set true and to false.
There was a problem hiding this comment.
Perfect. Thank you very much. Pushed it now.
|
Raaaaad! Nice one @slig! |
|
Great! Thank you very much! |
|
I forgot to add the Thanks and sorry! |
|
Any chance you can run a release and publish so that these type definitions are available for use? Thanks for adding them! |
|
These typings make default props being ignored in TS > 3.0. Anyone else also experiencing this issue? |
|
@feimosi could you please open an issue with a minimal reproduction? |
Using the definitions from #130 by @xaviergonz, I created the
react-sizeme.d.tsfile and updated thepackage.jsonto use them.Tested locally and it seems to work fine. Would appreciate someone more familiar with TypeScript bless this PR before merging it.
Thanks!