Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

[Feature] Create TypeScript typings #30

Closed
gregjopa opened this issue Oct 9, 2020 · 16 comments
Closed

[Feature] Create TypeScript typings #30

gregjopa opened this issue Oct 9, 2020 · 16 comments
Assignees

Comments

@gregjopa
Copy link
Contributor

gregjopa commented Oct 9, 2020

This project isn't written in TypeScript, but many TypeScript users would like to use published typings. For this issue, create TypeScript typings that cover the exported functions, and contribute it back to DefinitelyTyped!

This issue is great for someone who wants to learn more about TypeScript!

Here's an example from paypal-js: paypal/paypal-js#39

@sahaparamjit
Copy link

@gregjopa
Like to take this up

@gregjopa
Copy link
Contributor Author

@sahaparamjit thanks! I assigned it to you 😄

@PaulieScanlon
Copy link
Contributor

Hey @gregjopa and @sahaparamjit

What's the advantage of creating types and publishing them to DefinitelyTyped over converting the whole project over to TypeScript and bundling the types with the build?

@gregjopa
Copy link
Contributor Author

What's the advantage of creating types and publishing them to DefinitelyTyped over converting the whole project over to TypeScript and bundling the types with the build?

Great question @PaulieScanlon. We aren't ready to commit to using typescript in this repo. The thinking is adding types to DefinitelyTyped provides TypeScript developers with something useful. We are using this same strategy with several of the other paypal open source js projects.

@PaulieScanlon
Copy link
Contributor

@gregjopa Hey, yeah i understand where you're coming from DefinitelyTyped is a sensible middle ground. Perhaps when the time comes i could get back involved and see what i can do to help. I don't claim to be a TypeScript mega dude but i like using it and i'm always up for learning more.

@gregjopa
Copy link
Contributor Author

@PaulieScanlon sounds good! We appreciate your offer to help and will keep that in mind in our future typing adventures 😄

@camsjams
Copy link

Let me know if there is anything I can do to help @sahaparamjit

@gregjopa
Copy link
Contributor Author

gregjopa commented Nov 4, 2020

@camsjams would you be open to taking this on? I'm happy to assign it to you and help answer any questions.

The goal is to send a PR to DefinitelyTyped to add types for this library. The ScriptOptions are already typed from paypal-js so you could just copy and paste them: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/paypal__paypal-js/index.d.ts.

@camsjams
Copy link

camsjams commented Nov 4, 2020

Sure I can work on this, I have a couple of projects using this library that have local TypeScript PayPal definitions to make my code lint nicely, so it shouldn't be too terrible.

@gregjopa
Copy link
Contributor Author

gregjopa commented Nov 4, 2020

Awesome, thanks @camsjams! That's great to hear. No rush on this. Please tag me on the DefinitelyTyped PR.

@gregjopa
Copy link
Contributor Author

Hi @camsjams, I wanted to follow up. Are you still up for working on this? If not, are you able to share the type definitions here that you've been using internally? Thanks again.

@camsjams
Copy link

HI @gregjopa

I am planning on doing so this week, but just in case, I've uploaded the rudimentary one that I use in some projects here:
https://gist.github.com/camsjams/b843637df7a30c260b341daf97a96d60

Note: The above is mostly accurate, as the types are inferred from code and also the official PayPal SDK documentation, but I haven't done a thorough pass. It also only covers half of the React components in the library.

Like I said, I hope to get this done this week.

@gregjopa
Copy link
Contributor Author

I've uploaded the rudimentary one that I use in some projects here:
https://gist.github.com/camsjams/b843637df7a30c260b341daf97a96d60

Thanks for sharing @camsjams! This is a great start! 💯

It also only covers half of the React components in the library.

Feel free to skip the types for the <PayPalMessages /> component for now. It's still a WIP and we haven't documented the the prop types for it. The <PayPalMarks /> component is pretty simple with just one prop type of fundingSource

I hope to get this done this week.

Thanks again for taking this on. Later this week is totally fine.

@pedroapfilho
Copy link
Contributor

@gregjopa Wouldn't it be better to create an Umbrella issue, rewriting the lib to use Typescript? Maybe for version 4.

Doing so makes it easier to work with it, and it can lead to fewer bugs in the future.

@gregjopa
Copy link
Contributor Author

gregjopa commented Dec 4, 2020

@pedroapfilho sure, that's a great idea! Here's the umbrella issue: #69

@github-actions
Copy link

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

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

No branches or pull requests

6 participants