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

Port project to Typescript + Flow Types #805

Closed
3 tasks
MateusAndrade opened this issue Jun 17, 2020 · 13 comments
Closed
3 tasks

Port project to Typescript + Flow Types #805

MateusAndrade opened this issue Jun 17, 2020 · 13 comments

Comments

@MateusAndrade
Copy link
Collaborator

Describe the Feature

Since we have been doing a lot of changes to react-native-share with new documentation and improvements on the example application, aiming to make the life of any contributor easier.

Following the example of react-navigation which mainly uses TS now, it would simpler to update types, etc.

My idea is porting the main files which are currently written in plain .js with flow to ts. But maintaining the support for flow types. (Any examples are welcome)

Possible Implementations

Firstly, we need to map the common APIs exported from react-native-share and their respective/expected types. We can use the types from DefinitelyTyped as a start point.

After that, we only need to ensure that the compiled version of these files is working with both ts, flow, and js projects without any drawbacks.

Roadmap

  • Port main js files to TS
  • Ensure that both flow types and ts are supported
  • Compile project on CI before publish to npm
@mikehardy
Copy link
Collaborator

Another project I check went further without incident - just convert to typescript, evict the flow types and if people want flow-typing they can contribute types to flow-typed repo. It's basically reversing the previous stance (flow first / typescript secondary) but in a way that - while opinionated - recognizes the current javascript ecosystem reality. Typescript won.

@jgcmarins
Copy link
Member

That's great!
Check how graphql-js has been doing that.

@tcodes0
Copy link

tcodes0 commented Jun 17, 2020

👀 I can help with Ts but not so much with flow. I think the decision to go full TS is simpler than maintaining both TS and Flow. I'd still keep a branch with the flow types, just to have them as reference.

EDIT: what are the main js files to be ported to TS? these? https://github.com/react-native-community/react-native-share/tree/master/components

@MateusAndrade
Copy link
Collaborator Author

MateusAndrade commented Jun 17, 2020

👀 I can help with Ts but not so much with flow. I think the decision to go full TS is simpler than maintaining both TS and Flow. I'd still keep a branch with the flow types, just to have them as reference.

Sure! I'm a newbie with flow, maybe @jgcmarins can help us. I think I will push a new branch as soon as we start working on that to become our "backup". Do you have any thoughts about removing the types from DefinetlyTyped or something similar to that?

Edit:

Probably the components and mainly the index.js which is the root of the project that is a mess now. 😢

My idea is splitting the content inside this file into multiple options. Doing this would make the life of everyone easier, even a possible useShare would be simpler to create.

@mikehardy
Copy link
Collaborator

I believe you convert to typescript and the types in the repo become the canonical version at which you point you definitely remove the types from definitely typed. That's what I've seen

@aaronfg
Copy link

aaronfg commented Jun 17, 2020

Official types in TS would be great as the definitely typed defs are not up to date/correct. Especially with regards to what Options are from Share.shareSingle(options) vs Share.open(options).

Props on the Options marked as Optional in the Share.open docs don't match up with what's there in definitely typed, and there are missing items, etc.

I've patched the definitely type file for a while now to get rid of the linting errors. Having official TS defs would be fantastic.

@luancurti
Copy link
Contributor

Hey 👋🏻, I can help to migrate, I think we can enable the TypeScript (with a minimal configuration like this approach) in the project we can migrate file by file, after a full migration we can improve the configuration and compiler flags, WDYT?

@tcodes0
Copy link

tcodes0 commented Jun 18, 2020

Cool so I think we're looking for a WIP PR from someone to kickstart this. The main files to migrate would be

  • index.js
  • component/Button.js
  • component/Overlay.js
  • component/Sheet.js

let me know if I'm forgetting anything, kinda new to the project.

@MateusAndrade you mentioned refactors, I thought that maybe refactoring would be a different task. Typing first would make refactoring easier later because the compiler would help, just my 2cents here, I could be mistaken :)

btw index.js doesn't look that bad to me lol

@MateusAndrade
Copy link
Collaborator Author

Cool so I think we're looking for a WIP PR from someone to kickstart this. The main files to migrate would be

  • index.js
  • component/Button.js
  • component/Overlay.js
  • component/Sheet.js

let me know if I'm forgetting anything, kinda new to the project.

@MateusAndrade you mentioned refactors, I thought that maybe refactoring would be a different task. Typing first would make refactoring easier later because the compiler would help, just my 2cents here, I could be mistaken :)

btw index.js doesn't look that bad to me lol

Thanks! I agree with you. Actually we need to start with Typing and after that refactoring some implementations. 👯

@tcodes0
Copy link

tcodes0 commented Jun 29, 2020

Thoughts on how strict the ts config should be? would we use tsc to emit js or babel? Happy to get this started btw.

@mikehardy
Copy link
Collaborator

Possible starting points:
https://github.com/react-native-community/react-native-template-typescript/tree/master/template
https://github.com/react-native-community/react-native-webview
https://github.com/react-native-community/react-native-device-info/

I maintain device-info but the TS porters were others, who contributed the effort. That's important because I want to say that the produce is very easy to "drive" as the maintainer despite not having done it myself (though I do know typescript and my projects are in it)

One thing I want to mention as a library consumer is that it is very important to ship everything necessary to at least go in to node_modules and run a yarn/npm install and then a single build command which will result in any local changes then being used immediately. This "drive by hacking" is how a huge number of contributions / troubleshooting / bugfixing happen and you want to make that casual pre-fork edit experience easy

Cheers

@MateusAndrade
Copy link
Collaborator Author

@Thomazella I was thinking of starting this, but I won't have the appropriate time to focus on that. I would be happy on helping with anything. Also, on other projects, I was able to compile everything and use this with JS by running a tsc or something similar to that(I'm quite new with that).

@mikehardy react-native-device-info looks great. Maybe we can use this as a start point @Thomazella 🚀

@MateusAndrade
Copy link
Collaborator Author

Closing this prior to #1001! 🚀

@MateusAndrade MateusAndrade unpinned this issue Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants