search

Aljullu / react-lazy-load-image-component

React Component to lazy load images and components using a HOC to track window scroll position.
https://www.npmjs.com/package/react-lazy-load-image-component
MIT License
1.44k stars 109 forks source link

Add Typescript typings #34

Closed revskill10 closed 4 years ago

revskill10 commented 5 years ago

Description Add Typescript typings

nithincvpoyyil commented 5 years ago

Hi @Aljullu - just being curious, any plan to merge this PR?

Aljullu commented 5 years ago

Thanks for the PR @revskill10 and my apologizes for the delay answering. The fact is that my knowledge of TypeScript is almost 0, so I'm not sure what are the pros/cons of including this in the default package. I was planning to take a deep look at this PR at some point, but I have limited time to spend in this project, so I have been delaying it more than I initially thought.

With a quick look at the code, one of the questions I get is whether we should also include interface LazyLoadComponentProps in addition to interface LazyLoadImageProps.

cc @nithincvpoyyil

rodrigomf24 commented 4 years ago

It would be great to have support for Typescript ❤️ I am willing to help

nithincvpoyyil commented 4 years ago

Thanks for the PR @revskill10 and my apologizes for the delay answering. The fact is that my knowledge of TypeScript is almost 0, so I'm not sure what are the pros/cons of including this in the default package. I was planning to take a deep look at this PR at some point, but I have limited time to spend in this project, so I have been delaying it more than I initially thought.

With a quick look at the code, one of the questions I get is whether we should also include interface LazyLoadComponentProps in addition to interface LazyLoadImageProps.

cc @nithincvpoyyil

@Aljullu : Yes, its better to do in one shot.
@revskill10 : Could you please update that as well, in this PR?

danvk commented 4 years ago

@nithincvpoyyil @revskill10 @Aljullu FYI another option for TypeScript declarations is to host them separately on DefinitelyTyped. There are pros & cons to bundling the typings with this repo vs. externally, but if you're not familiar w/ TS and find the prospect of maintaining type declarations daunting, it might be a good option.

If you'd prefer that approach, I'm happy to create a PR on DT and get it through.

Thanks for the great library!

Aljullu commented 4 years ago

If you'd prefer that approach, I'm happy to create a PR on DT and get it through.

That would be great @danvk! As you said, I'm a bit reluctant to add TypeScript declarations in this repo because I won't be able to maintain them and wouldn't like to merge something that might block future releases, so hosting them in DefinitelyTyped sounds like a good solution to me.

Let me know if I can help in any way!

danvk commented 4 years ago

Here it is: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/40565

@revskill10 if you could look at the type declarations, that'd be great. I've listed you as an author / maintainer.

@Aljullu I've tried to figure out expected usage for things like trackWindowScroll but I'm not entirely sure if I got it right. It would be great if you could look at the tests.

danvk commented 4 years ago

... and they're merged. You can get type declarations by running:

npm install -D @types/react-lazy-load-image-component
Aljullu commented 4 years ago

Thank you @danvk! I added a link in the README so everybody can find them:

https://github.com/Aljullu/react-lazy-load-image-component#features

nithincvpoyyil commented 4 years ago

@danvk :+1: