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.

Sign up for GitHub

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

Jump to bottom

feat(typescript): migrate project to typescript #1001

Merged
merged 7 commits into from Apr 19, 2021
Merged

feat(typescript): migrate project to typescript #1001

merged 7 commits into from Apr 19, 2021

Conversation

DanielMarkiel
Copy link
Contributor

Overview

Introduces typescript to project

See #805 and #818

@tcodes0 Thanks for your input!

Test Plan

Run yarn validate

TODO

  • deps and config
  • components
  • eslint-typescript configuration, should fix ci
  • figure out how to build ts for npm
  • type index.js
  • remove flow
  • read bridge and double check types

EstebanFuentealba
EstebanFuentealba reviewed Apr 16, 2021
View reviewed changes
ios/RNShare.m
@@ -104,12 +104,12 @@ - (NSDictionary *)constantsToExport
{
return @{
@"FACEBOOK": @"facebook",
@"FACEBOOK_STORIES": @"facebookstories",
@"FACEBOOKSTORIES": @"facebookstories",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To standardize naming between iOS and Android.
In android we've got facebookstories (which is later uppercased to FACEBOOKSTORIES).
Original implementation -> NativeModules.RNShare.FACEBOOK_STORIES was returning undefined on Android (because the key name there is FACEBOOKSTORIES).

EstebanFuentealba
EstebanFuentealba reviewed Apr 16, 2021
View reviewed changes
prettier.config.js
@@ -1,5 +0,0 @@
module.exports = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if deleting prettier.config.js is a good idea. Because in future changes the code will be indented with the configuration of each code editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configuration was moved to .eslintrc.js file.
I've got a prettier plugin in my editor and it was recognizing the config correctly. After this change yarn lint will let you know if something is wrong even if you don't have a plugin. Also CI (yarn validate) will be double-checking that everything is ok.

@MateusAndrade
Copy link
Collaborator

What about keeping the flow types @DanielMarkiel? How do you think we would be able to achieve this or do you think it's worth it to keep these types?

@DanielMarkiel
Copy link
Contributor Author

@MateusAndrade I agree with the @mikehardy 's opinion presented here -> #805 (comment).
If there's a need for flow types the best way would be to put them into flow-typed repo.
It should be fairly easy now when the package is typed in typescript (first tool from the google search -> https://github.com/joarwilk/flowgen)

@EstebanFuentealba
Copy link
Collaborator

EstebanFuentealba commented Apr 16, 2021 •
edited

Good job, maybe it would be good to add an extension to the definition of NativeModules like this

declare module 'react-native' {
  export interface RNShare {
    open: (config: ShareOptions, /* TODO: ... */) => Promise<void>;
    /* TODO */
  }
  interface NativeModulesStatic {
    RNShare: RNShare;
  }
}

@DanielMarkiel
Copy link
Contributor Author

@EstebanFuentealba Great suggestion! :) Done

@MateusAndrade
Copy link
Collaborator

Nice!!! Do you guys think we should bump this as a major version?

cc: @EstebanFuentealba , @DanielMarkiel

@DanielMarkiel
Copy link
Contributor Author

Nice!!! Do you guys think we should bump this as a major version?

cc: @EstebanFuentealba , @DanielMarkiel

Yep, in my opinion it should be a major version bump

MateusAndrade
MateusAndrade previously approved these changes Apr 16, 2021
View reviewed changes
@MateusAndrade
Copy link
Collaborator

Do you think we can go on with this @EstebanFuentealba?

MateusAndrade
MateusAndrade reviewed Apr 17, 2021
View reviewed changes
src/types.ts
}

export interface FacebookStoriesShareSingleOptions extends BaseSocialStoriesShareSingleOptions {
social: Social.FacebookStories;
method: FacebookStories;
method: Exclude<ShareAsset, ShareAsset.BackgroundVideo>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@MateusAndrade
Copy link
Collaborator

@DanielMarkiel did you test this with the example app?

@DanielMarkiel
Copy link
Contributor Author

@MateusAndrade Yes, I did. Android looked fine but I couldn't test the iOS version properly since I don't have an iPhone.
I would appreciate it if someone did a retest and double-checked that the package is still working as expected.

@MateusAndrade
Copy link
Collaborator

@MateusAndrade Yes, I did. Android looked fine but I couldn't test the iOS version properly since I don't have an iPhone.
I would appreciate it if someone did a retest and double-checked that the package is still working as expected.

Tested here with iOS and it's working! 🚀

I'm going to merge this and publish a major version. Thanks a lot, @EstebanFuentealba, @DanielMarkiel!!

@MateusAndrade MateusAndrade enabled auto-merge (squash) April 19, 2021 23:28
@MateusAndrade MateusAndrade merged commit c2085d3 into react-native-share:master Apr 19, 2021
MateusAndrade added a commit that referenced this pull request Apr 19, 2021
@semantic-release-bot @MateusAndrade
chore(release): 6.0.0 [skip ci]
4cc20d3 
# [6.0.0](v5.3.0...v6.0.0) (2021-04-19)

### Features

* migrate project to typescript ([#1001](#1001)) ([c2085d3](c2085d3))

### BREAKING CHANGES

* migrate the project to typescript

* feat(typescript): migrate project to typescript

* feat(typescript): extend react native NativeModules interface with RNShare types

* feat(typescript): remove unnecessary 'exclude' clause in tsconfig

* feat(typescript): fix ts error when using RNShare.Facebook/InstagramStories

Co-authored-by: Mateus Andrade <15278828+MateusAndrade@users.noreply.github.com>
@MateusAndrade
Copy link
Collaborator

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MateusAndrade MateusAndrade mentioned this pull request Apr 19, 2021
Closed
7 tasks
@DanielMarkiel DanielMarkiel deleted the typescript branch April 20, 2021 06:12
@MateusAndrade MateusAndrade mentioned this pull request Apr 20, 2021
Closed
3 tasks
mobiledev7 added a commit to mobiledev7/react-native-share that referenced this pull request Dec 12, 2022
@mobiledev7 @MateusAndrade
chore(release): 6.0.0 [skip ci]
5116095 
# [6.0.0](react-native-share/react-native-share@v5.3.0...v6.0.0) (2021-04-19)

### Features

* migrate project to typescript ([#1001](react-native-share/react-native-share#1001)) ([c2085d3](react-native-share/react-native-share@c2085d3))

### BREAKING CHANGES

* migrate the project to typescript

* feat(typescript): migrate project to typescript

* feat(typescript): extend react native NativeModules interface with RNShare types

* feat(typescript): remove unnecessary 'exclude' clause in tsconfig

* feat(typescript): fix ts error when using RNShare.Facebook/InstagramStories

Co-authored-by: Mateus Andrade <15278828+MateusAndrade@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MateusAndrade MateusAndrade MateusAndrade approved these changes

@jgcmarins jgcmarins Awaiting requested review from jgcmarins

@mikehardy mikehardy Awaiting requested review from mikehardy

@EstebanFuentealba EstebanFuentealba Awaiting requested review from EstebanFuentealba

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DanielMarkiel @MateusAndrade @EstebanFuentealba