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

Maintain our own ts definitions? #545

Open
dboskovic opened this issue Jul 28, 2018 · 13 comments
Open

Maintain our own ts definitions? #545

dboskovic opened this issue Jul 28, 2018 · 13 comments

Comments

@dboskovic
Copy link
Collaborator

dboskovic commented Jul 28, 2018

I think it might be a good idea to move our TS definition file into the core repo and maintain our own type file / updating it as part of PRs. Right now we rely on the DefinitelyTyped contributions which are already out of date.

Any thoughts @pokoli @mholt?

Current TS definitions maintained here: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/papaparse

@pokoli
Copy link
Collaborator

pokoli commented Jul 30, 2018

@dboskovic sorry if the question is quiet obious but I will like to know why the TS Definition file is required and for what it is issued. Sincerly I did not know that this exists.

@dboskovic
Copy link
Collaborator Author

It's not required unless you're a Typescript user. Then you need type definitions. So the DefinitelyTyped project allows people to contribute the types for libraries that don't provide them as part of the project. TS is becoming very popular so it makes sense to me that we would maintain the type file with our library rather than leaving it separate for others to maintain.

@pokoli
Copy link
Collaborator

pokoli commented Jul 30, 2018

I do not think we should force all contributors to update them as it can be people that use PapaParse with plain Javascript (I'm one of them).

@dboskovic
Copy link
Collaborator Author

Hmmm, I guess we could just conduct a review of the DefinitelyTyped definitions from time to time and make sure they're up to date.

@pokoli
Copy link
Collaborator

pokoli commented Aug 1, 2018

Probably it's enought to update it for each new PapaParse version

@wartab
Copy link

wartab commented Mar 28, 2019

A type definition file also has advantages for plain JavaScript users as IDEs like WebStorm will still make use of them to help autocompleting when not just doing TypeScript. I think that you should really add this here, even if some contributors will not bother filling it in. It's easier to remember editing it directly in the concerned package, too.

@pokoli
Copy link
Collaborator

pokoli commented Mar 28, 2019

It is possible to generate/update type definitions based on a script?
If so we may problably include this on our release process

@bforbis
Copy link

bforbis commented Jul 13, 2019

The only easy way to generate type definitions is to convert your project to Typescript. It just pulls the definitions directly from your codebase. If that is too big of a lift or undesirable, you could just have a separate type declaration file in your repo that you maintain on the side, but you would need to write some tests in typescript to make sure they work.

@crhistianramirez
Copy link

crhistianramirez commented Nov 2, 2023

+1 for a type definition file that lives with the project.

Pros:

  • Better type support for both typescript users and non-typescript users. Modern IDEs use the type information to enhance the developer experience even if you're just using Javascrit
  • Keeping types in the project means the type definitions will always remain in sync with the source code files
  • You don't need to install a separate type definition package

Cons:

  • Have to maintain the type definition files. Recommendation would be to update the source code to use typescript so that those type definition files are generated directly from the code. Only drawback is you'd have to update the source code to typescript but given its a single 2,000 line file it seems pretty feasible.

@dboskovic
Copy link
Collaborator Author

I'd be anxious about rocking the boat here too much in a typescript refactor of the existing code. It's not just typing, but different generated code and possibly some things to optimize.

I think this library is due for an overall rewrite though - and if we did that, I'd be in full support of typescript.

@pokoli
Copy link
Collaborator

pokoli commented Nov 2, 2023

Just to be honest, I'm not fan of rebuilding the library from scratch and also I'm not a TypeScript developer because javascript is not my primary language (I mainly work on Python). So don't count for me for start the TypeScript migration.

Having said that, if somebody wants to start the port in a fork, I will be happy to review them and if this really improves the current codebase merge it on a new major version.

@dboskovic
Copy link
Collaborator Author

dboskovic commented Nov 2, 2023

I'll talk to our team and see if we want to invest in a refactor. But I'm guessing that the original proposal of just bringing the type definitions home to this repo is still the most reasonable scope of work for the community to do.

@pokoli
Copy link
Collaborator

pokoli commented Nov 2, 2023

@dboskovic We can bring the definitions where if you prefer but I can not guarantee to keep them updated unless there is some script that I can include in the release process to create new types. And I will prefer to have nothing that having something not updated.

I do not think there will be such much new types added to public functions. Probably I can ask on MR to include the TS definitions also. It will be great to get some way of CI process to fail if there are missing definitions. Do you know if there is some way to check pragmaticly that all TS definitions are updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants