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

Open letter - Road to 7.0.0 #16

Closed
2 of 4 tasks
thebergamo opened this issue Apr 14, 2021 · 19 comments
Closed
2 of 4 tasks

Open letter - Road to 7.0.0 #16

thebergamo opened this issue Apr 14, 2021 · 19 comments
Labels
enhancement New feature or request help wanted Extra attention is needed wontfix This will not be worked on

Comments

@thebergamo
Copy link
Owner

thebergamo commented Apr 14, 2021

I decided to create this issue to have the plans about the future of this library in a way that others could help and not only wait for me or any other maintainer.

Also feel free to review any PR that is open or engage into the issues and that also needs some help either helping with questions, triage, or anything. I will be more than happy to include you as contributor and maintainer in this project if you would.

I see the future of this library moving towards a full typescript migration and a split the repository (monorepo into smaller parts of the FB SDK library, as the native dependencies are already splitter so we could also split it into specific libs under the same umbrela in order to improve the code splitting and saving for the app consumers of this library. (which leads us to smaller apps)

So, at this moment the main steps from my perspective are:

If you see any other major step that you believe should be included into this 5.0.0 milestone please comment below ;)

@thebergamo thebergamo added enhancement New feature or request help wanted Extra attention is needed labels Apr 14, 2021
@iduuck
Copy link
Contributor

iduuck commented Apr 15, 2021

For point "Split the repository in a monorepo (using lerna (?))" I would say, it would make sense to have something like the core (see rn-firebase), and then multiple things on top, in my eyes, lerna makes completely sense.

@mikehardy
Copy link
Collaborator

rn-firebase maintainer here 👋 😅 - yes lerna has been a good experience

you will definitely want a core / central / always-installed package to share some infrastructure and versioning etc

a monorepo is the only way I know of to handle underlying library splits well so it does seem like a good fit

@thebergamo
Copy link
Owner Author

I agree! Need to take a deeper look into rn-firebase:)

@kmsbernard
Copy link
Contributor

kmsbernard commented Apr 15, 2021

Although it is not directly related with the functionality, detailed & user-friendly documentation will be big help.

@g-dimitry
Copy link
Collaborator

About the migration to TS. I could start now but i guess this will break the deployment process (is it a script or is it manual ?)

@thebergamo
Copy link
Owner Author

It's automated by github actions.
That's fine if you broke it we can fix it later on :)

@mikehardy mikehardy changed the title Open letter - Road to 5.0.0 Open letter - Road to 7.0.0 Nov 15, 2021
@mikehardy
Copy link
Collaborator

Typescript has a PR in progress but looks like the PR needs someone to pick it up and give it some polish so it is merge-able
SDKs are current now!
Updated description above

@stale
Copy link

stale bot commented Dec 15, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Dec 15, 2021
@rnike
Copy link
Collaborator

rnike commented Dec 17, 2021

TypeScript migration #155

@stale stale bot removed the wontfix This will not be worked on label Dec 17, 2021
@mikehardy
Copy link
Collaborator

@rnike you win all my internet points for the day for doing that PR, thank you 🏆

@mikehardy
Copy link
Collaborator

Typescript is out as @beta tag on npmjs.com

formal version is v7.0.0-beta.1 on npmjs.com and on the beta branch, got it all working.

Now it can incubate - and this is a call for testers, any interested parties should test this in their projects and report problems before end of first week of January!

@rnike --> 🏆 !

@rnike
Copy link
Collaborator

rnike commented Dec 29, 2021

Thank you @mikehardy for all the help, finally we can install beta from npm, the AEM logger(#165) is still in need for us to actually test it on the production app, not meant to be in hurry but reviews are welcome

@mikehardy
Copy link
Collaborator

Okay, I will prioritize that one. Can merge it into beta branch easy I think

@thebergamo
Copy link
Owner Author

Amazing! First week of January I've to implement a new feature in my app related to Facebook and will be able to test the beta on iOS will put my feedback here :)

Next step would be start splitting into modules.

Any guidance to have something similar to rn-firebase is very welcomed @mikehardy

@mikehardy
Copy link
Collaborator

My best guidance is don't, unless you really really need to. Multimodule monorepos are kind of a pain in the butt haha. When you can't avoid them, then sure, but if there is not a clear reason I wouldn't...

@rnike
Copy link
Collaborator

rnike commented Jan 4, 2022

Checked the typescript checkbox, it's done in #155

@gvillenave
Copy link

+1 for splitting into multiple packages and separate Core/Login/Share, one issue with iOS App Store submission for example is that even if you only use FBSDKLoginKit the app still is considered using Photo Library and Microphone access (for which you have to provide justifications for), because the APIs are referenced by FBSDKShareKit. So it would be definitely nice to be able to only consume the packages you need :)

@mikehardy
Copy link
Collaborator

Permission splitting for app store approval is one of the valid reasons to have multiple packages @gvillenave, so that's a good point. There are still other ways to do it perhaps than a mono-repo, something worth mentioning - for instance on the iOS side react-native-permissions has you include more targetted podspec files https://github.com/zoontek/react-native-permissions - however that puts a requirement on the user to include what they want at the native level (in podspec) vs allowing auto-linking to "just handle it". So there are still non-monorepo ways to do it but they involve tradeoffs between native vs javascript implying different choices about who should do the work (monorepo -> maintainers do the work, auto-linking handles it for devs, podspec -> integrating users do the work, cocoapods handles it)

I'd go for monorepo in that case even though it's a maintainer pain. In the long run the integration effort results in extra issues from confused users that adds up over time and is synchronous (more irritating, for maintainers) vs async maintainer work

@stale
Copy link

stale bot commented Feb 7, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Feb 7, 2022
@stale stale bot closed this as completed Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

7 participants