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

Feat/ts #818

Closed
wants to merge 16 commits into from
Closed

Feat/ts #818

wants to merge 16 commits into from

Conversation

tcodes0
Copy link

@tcodes0 tcodes0 commented Jul 5, 2020

Overview

see #805
Introduces typescript to project

Test Plan

run yarn validate:ts

todo

  • deps and config
  • components/
  • eslint-typescript configuration, should fix ci
  • figure out how to build ts for npm
  • type index.js (maybe in another pr)
  • flow: remove?
  • read bridge and double check types

@tcodes0
Copy link
Author

tcodes0 commented Jul 5, 2020

Do we want TS to emit types and declarations or just type-check?

package.json Show resolved Hide resolved
@mikehardy
Copy link
Collaborator

Ohhh 🤤
Your question on emitting types and declarations: I think one of the stated goals of the conversion is to be the source of truth for typing (so that people don't need to import from definitely typed). In order to do that we must necessarily emit the types and declarations I think?

.eslintrc Show resolved Hide resolved
package.json Show resolved Hide resolved
@tcodes0
Copy link
Author

tcodes0 commented Jul 7, 2020

@MateusAndrade on a good spot for a review, specially around the ts-ignore stuffs

index.tsx Show resolved Hide resolved
index.tsx Show resolved Hide resolved
index.tsx Outdated Show resolved Hide resolved
index.tsx Outdated Show resolved Hide resolved
index.tsx Show resolved Hide resolved
index.tsx Show resolved Hide resolved
index.tsx Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Co-authored-by: luancurti <luancurti@gmail.com>
Copy link
Collaborator

@MateusAndrade MateusAndrade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @Thomazella. I have some considerations about splitting the types from the main file into different files, something like types.d.ts or index.d.ts. By doing this we would need to export the types letting this easier to use outside the library.

Also, we don't need to declare a namespace or something similar to that? 🤔

components/Button.tsx Outdated Show resolved Hide resolved
index.tsx Outdated Show resolved Hide resolved
index.tsx Outdated Show resolved Hide resolved
index.tsx Outdated Show resolved Hide resolved
@tcodes0
Copy link
Author

tcodes0 commented Jul 12, 2020

I'm hoping we can emit that file using the source code as base.

@luancurti luancurti linked an issue Jul 12, 2020 that may be closed by this pull request
3 tasks
@mikehardy
Copy link
Collaborator

Yep - device-info emits the file for what it's worth, using bob as a build tool it just happens?
I'm not really watching this closely so nothing just wanted to say in general, nice work :-), I'm excited to see the result

@tcodes0
Copy link
Author

tcodes0 commented Jul 14, 2020

So the last thing is to build the js with tsc, and double check types. I think we'll get the .d.ts files "for free" like that, which should make it easier to maintain. I think it may make sense to type the bridge stuff too, but we'd probably need to do that in a file and maintain it. Maybe another PR for that. Overall we're close.

@mikehardy
Copy link
Collaborator

All of that should be for free with bob I think here is the stanza from device info https://github.com/react-native-community/react-native-device-info/blob/master/package.json

  "@react-native-community/bob": {
    "source": "src",
    "output": "lib",
    "targets": [
      "commonjs",
      "module",
      "typescript"
    ]
  }

@tcodes0
Copy link
Author

tcodes0 commented Jul 15, 2020

@mikehardy ok, didn't know bob, I get it now. Thanks!

@luancurti
Copy link
Contributor

@Thomazella what's the status of this PR? Do you need some help?

@tcodes0
Copy link
Author

tcodes0 commented Sep 8, 2020

@luancurti nope, should be able to finish, will aim for this week. I was just busy with my new job that's all.

@MateusAndrade
Copy link
Collaborator

@luancurti nope, should be able to finish, will aim for this week. I was just busy with my new job that's all.

Let me know if you need any help. 👯

@DanielMarkiel
Copy link
Contributor

@MateusAndrade , @tcodes0
What's the status of this PR? Is it stale (nothing has changed since Jul 13, 2020)?
If yes, would you mind if I try completing it next week?

@tcodes0
Copy link
Author

tcodes0 commented Apr 9, 2021

feel free to complete it :)

@MateusAndrade
Copy link
Collaborator

@MateusAndrade , @tcodes0
What's the status of this PR? Is it stale (nothing has changed since Jul 13, 2020)?
If yes, would you mind if I try completing it next week?

Let me know if there is anything we can do to help. 🚀

@MateusAndrade
Copy link
Collaborator

Thanks a lot, guys, we merged #1001 and successfully published it under the version v6.0.0. 🚀

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

Successfully merging this pull request may close these issues.

Port project to Typescript + Flow Types
6 participants