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

Migrate library to Typescript #25

Closed
wants to merge 6 commits into from
Closed

Migrate library to Typescript #25

wants to merge 6 commits into from

Conversation

MichaelSolati
Copy link
Contributor

It's a full conversion of the Typeform JS SDK to Typescript for use of types/interfaces. Wether writing code in TS or JS some IDEs will pick up on the definitions and suggest completion and explain what functions do (like below).

screen shot 2018-11-28 at 1 38 40 pm

I just want to go through the typings of responses to fix up some things, and make them easier for developers to use, but the code itself is good. I updated the tests to support Typescript. I updated the build to use rollup just because I was more familiar with it when using Typescript. I also did some other minor tweaks to linting for Typescript, but that shouldn't be that big of a deal. Hopefully everything besides the interface stuff is good to review.

@picsoung
Copy link
Contributor

picsoung commented Dec 4, 2018

Nice one @MichaelSolati!

ping @jepser what do you think?

@jepser
Copy link
Contributor

jepser commented Dec 5, 2018

@MichaelSolati thanks for your contribution and sorry for my delayed answer, I have a couple of comments I would like to share with you:

  • If I understand the motivation to use Typescript was to create interfaces and ensure the correct use of the methods available by forcing types on args, which I think is great, therefore I think we could go with a softer approach, we could add js docs, which you already did, and then add TS to ensure the correct types for methods.
  • I would like to know what's the benefit on changing the build to rollup, I don't see the value if it's just more familiar for you
  • I'm curious why changing the API, which is the main motivation and benefit

Finally, I believe we could break down this PR in simpler (and smaller) ones.

@MichaelSolati
Copy link
Contributor Author

@jepser it's not a problem, I know that this time of year is hectic for everyone, so I get it 100%. Anyway, addressing your points...

  • Breaking up these changes into a slower and more gradual process is cool with me
  • So I can move the build from Rollup back to Webpack, but I do think the Babel can be omitted for the Typescript compiler (which I believe is used for the lib build). I do think I'd need a little more insight as to how the two builds work in order to better address this.
  • I believe I changed how things worked under the hood in order to better resemble a Class/Object. The return of just a JSON object wouldn't have been super friendly for using intellisense. I did try to ensure that everything function matched up though, so the only difference to the end user was how they instantiated the client, otherwise every function and call behaves exactly the same.

@jepser jepser added the invalid This doesn't seem right label Dec 8, 2018
@jepser
Copy link
Contributor

jepser commented Dec 8, 2018

Thanks for the explanation @MichaelSolati , wdyt about:

  1. Splitting the PR in smaller relative parts, we could start with creating one with JS Docs so we can have args hinting
  2. Let's create an issue about TS, writing the motivation (or create a PR, but I would recommend the first option)

As recommendations, we won't change the interface since we already have people using it and I don't see the benefit of it, changing it will also create a breaking change version. Also, we won't change webpack to rollup unless there is a real benefit, which I'm open to discuss in an issue.

I will tag this PR for reference and close it once we have the steps above done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants