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(app): migrate app module to a modular API #6694

Merged
merged 8 commits into from
Dec 5, 2022
Merged

feat(app): migrate app module to a modular API #6694

merged 8 commits into from
Dec 5, 2022

Conversation

Lyokone
Copy link
Contributor

@Lyokone Lyokone commented Nov 14, 2022

Description

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Nov 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Dec 5, 2022 at 1:27PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Dec 5, 2022 at 1:27PM (UTC)

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #6694 (cb8415c) into main (3c8e6ba) will decrease coverage by 0.02%.
The diff coverage is 37.50%.

❗ Current head cb8415c differs from pull request most recent head e2388e1. Consider uploading reports for the commit e2388e1 to get more accurate results

@@             Coverage Diff              @@
##               main    #6694      +/-   ##
============================================
- Coverage     54.04%   54.03%   -0.01%     
  Complexity      690      690              
============================================
  Files           216      217       +1     
  Lines         10782    10790       +8     
  Branches       1698     1699       +1     
============================================
+ Hits           5826     5829       +3     
- Misses         4664     4668       +4     
- Partials        292      293       +1     

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Seems about right - and kind of exciting if it's really this small

@Ehesp what do you think about providing stubs for the APIs we have gaps on for platform reasons? (onLog, registerVersion) to close the JS -> RNFB API gap completely?

https://firebase.google.com/docs/reference/js/app

@Lyokone
Copy link
Contributor Author

Lyokone commented Nov 14, 2022

Yeah didn't knew what to do about those, but I can definitely add those 👍

@mikehardy
Copy link
Collaborator

I think it depends on the goal, and I'm not sure we have a defined goal there which is why I did an @ for Elliot.
If our goal is for people to not have to do a bunch of if (Platform.....) then at least stubbing them might be nice. I personally think this would be nice.

But maybe being explicit that they are not supported is better, and making it explicit by not implementing them, so people have to Platform is "safest" ? I personally don't think that's useful in practice but again I could be missing something.

So my opinions are weakly held on stubbing the non-native APIs but I lean towards the idea stubbing them and eliminating all need for Platform checks would be cool

@Lyokone
Copy link
Contributor Author

Lyokone commented Nov 17, 2022

The coverage bot is saying that some lines are not tested even thought the test was properly added.

Copy link
Collaborator

@mikehardy mikehardy 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 pretty good - I had a technical comment on failure assertions but that's trivial.

The only structural thought I have is:

  1. IF all the new stuff is "v9" and "modular",
  2. AND we assume that eventually v8 JS API goes away (I assert it will, new APIs are already v9-only
  3. THEN we will eventually deprecate and remove v8
  4. meaning that there will be a second big cleanup phase here to shift v9 to "default" and purge v8

I assert that we can avoid the second big cleanup phase (or make it trivial) if we build v8 obsolescence into these PRs.

What do I mean? I mean, what if instead of importing from v9 and exposing "modular", the first step of the PR was to move v8 implementation + tests to v8 sub-directories, and import it as "compat" (similar to the way you access the v8 APIs on the JS SDK now).

That could - I think - still just be internal structure, in our own exports (or re-exports) all the symbols would still be there so consumers would not have to think about it

But in the final state where the v8 / chained stuff likely goes away based assertion 2 above, we don't have a painful future cleaning phase, since we made v9 the default structurally as we were going through

Thoughts?

packages/app/e2e/app.v9.e2e.js Outdated Show resolved Hide resolved
@Lyokone Lyokone changed the title feat(app): migrate app module to v9 feat(app): migrate app module to a modular API Nov 23, 2022
@Lyokone Lyokone changed the title feat(app): migrate app module to a modular API feat(app): migrate app module to a modular API Nov 23, 2022
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - I don't want to delay any longer sorry it sat this long!

@mikehardy mikehardy merged commit c285016 into main Dec 5, 2022
@mikehardy mikehardy deleted the feat/v9-app branch December 5, 2022 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants