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

WIP: Add typescript typings #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

renestalder
Copy link

Allows to use the scrollMonitor library in TypeScript projects.

Started using the scrollMonitor library in a typescript project and wrote typings to get it working. Calling this a WIP as long I didn't test all the functions and properties, and also because I never published any typings yet.

Also, I'm not sure this has to go to the maintainers repository or to the typings repository.
There's a recommendation, but IMHO it doesn't state that one or the other method is a must.

@stutrek
Copy link
Owner

stutrek commented Mar 14, 2018

I don't use typescript so I can only review so much, but it looks right to me.

However, it says on the page you liked to that if the project is not written in Typescript then publishing to the @types namespace is preferred. That seems strange to me, but I think it's so that you know which version of scrollmonitor has types. If I was to change the library it's likely that I would completely forget to update the types, or mess up the types file if I tried.

@renestalder
Copy link
Author

Yeah, not sure yet about that part. Will check some other projects. Seems so much more intuitive to have the typings inside the project because this means the developer can skip manual typing installation. But the argument for versioning seems fair.

I'll check. For the moment, I wouldn't recommend to merge, even those typings seems to work so far.
Just keeping this here as a reminder.

@stutrek
Copy link
Owner

stutrek commented Mar 14, 2018

Even if we never merge it we should keep the PR open so anyone using TypeScript can +1 it and use it from your fork.

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.

2 participants