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

fix(database): fixed typescript type definitions #4550

Closed
wants to merge 2 commits into from

Conversation

SCasarotto
Copy link

Description

See Discussion in #4545 for context. Simply fixing up some typescript type definitions to match the js lib.

Related issues

Fixes #4545

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:

🔥

@CLAassistant
Copy link

CLAassistant commented Nov 13, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Stuart Casarotto seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vercel
Copy link

vercel bot commented Nov 13, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/pri356v41
✅ Preview: https://react-native-firebase-git-master.invertase.vercel.app

@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #4550 (fc80063) into master (a1e90ae) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4550   +/-   ##
=======================================
  Coverage   69.10%   69.10%           
=======================================
  Files         109      109           
  Lines        3699     3699           
  Branches      276      276           
=======================================
  Hits         2556     2556           
  Misses       1041     1041           
  Partials      102      102           

@mikehardy
Copy link
Collaborator

Following on the discussion from the issue, do you mind if I peg this one as pending on resolution of your upstream documentation request, to make sure we don't waffle on the types if they go a different direction? (you never know!)

In the meantime, so that isn't an inconvenient stance for you I highly recommend https://github.com/ds300/patch-package if you haven't seen it, and you should note that we generate patches for every PR so they are easy to integrate + test :-), just follow the "details" link above on the 'Create Test Patches' check and you'll find a little hard-to-see dropdown for "artifacts" where you can snag the zip file with the patches. Or obviously you can make them yourself

@mikehardy mikehardy added blocked: firebase-support Pending feedback or review from google support or response on official sdk repo issue. plugin: database Firebase Realtime Database tools: typings TypeScript / Flow Type: In Progress Workflow: Waiting for User Response Blocked waiting for user response. labels Nov 13, 2020
@SCasarotto
Copy link
Author

Yeah I think thats totally fine. This isn't really a blocker and I can easily locally override the types. Having said that, ultimately its their docs site and not their working code that is at fault, so I would be surprised my request there has any impact on the actual js lib. Ultimately no rush.

Have been using this lib for years (ever since the v5/v6 conversion) and am just glad I get to contribute. 😄 🚀

@SCasarotto
Copy link
Author

SCasarotto commented Jan 14, 2021

That has now been merged into the firebase docs have been updated thus this PR should be allowed to be merged. It does look like there is now a merge conflict. It looks like there have been changes to the types slightly. These types that are currently merged don't directly reflect the firebase docs (which I thought was our standard here). Below are the differences:

Current Master Code:

      callback?: (data: DataSnapshot, previousChildKey?: string) => void,
      cancelCallbackOrContext?: Record<string, any>,
      context?: Record<string, any> | null,
    ): () => void;

My Code:

      callback?: (a: DataSnapshot, b?: string | null) => any,
      cancelCallbackOrContext?: ((a: Error) => any) | Record<string, any> | null,
      context?: Record<string, any> | null,
    ): (a: DataSnapshot, b?: string | null) => any;

Firebase Code (ref):

    callback: (a: DataSnapshot, b?: string | null) => any,
    cancelCallbackOrContext?: ((a: Error) => any) | Object | null,
    context?: Object | null
  ): (a: DataSnapshot, b?: string | null) => any;

As I am not deeply in tune with this community's thoughts I don't know if I am the right person to resolve these conflicts. I think it can make sense to name the variables something more meaningful. There doesn't seem to be a lot of harm in that. However, some of the actual types are different and that seems to go against trying to keep them in sync.

Any thoughts?

Other Context:

@mikehardy
Copy link
Collaborator

The type deviation was a result of me doing #4711 a large-scale depedency update that flagged a ton of lint and then the corresponding de-linting pass where eslint typescript rules were complaining about the 'Object' and 'Function' definitions - 1beb5b5?diff=unified&w=1#diff-bf8976d56b60a0e9ceb9258c0027de145b69fa134f8ec1bde7701ffb6788a9a9

The firebase-js-sdk typings should be our standard, my intention is not to deviate from them but I dropped the ball on this one

I was moving quickly through there and completely forgot I was constrained by firebase-js-sdk API typing 🤦 - at the same time I was referring to the actual javascript implementation as I was modifying the types, so I'm not sure we would accept or generate the full set of firebase-js-sdk type signature options for some of the parameters / return values etc.

I have these goals then

  • I need yarn lint to pass, and I can accept tsconfig changes or ts-ignore directives if needed to make it so
  • I don't want things added to the signature that don't appear to be valid in our codebase - even if that represents a slight deviation (like, some of the possible parameter types in a positional argument aren't valid for us
  • I do want any types we do accept to match firebase-js-sdk if at all possible

I think that gives us the best possible outcome: we'll fail to compile things that aren't valid here but it will be possible to write code that is perfectly portable between react-native-firebase and firebase-js-sdk

I'm running a little low on sleep - so to confirm: Does that sound reasonable? Do you think a mild re-shape of this PR while de-conflicting can meet all those goals, at least for this function? (possibly for the file, if you're feeling ambitious)

@SCasarotto
Copy link
Author

@mikehardy This totally makes sense. Thanks for the detailed reply. There is a lot on my plate for the next couple weeks but will try to circle back to help on this stuff.

@mikehardy
Copy link
Collaborator

Hey @SCasarotto no pressure, I understand contributing to open source projects does not frequently move to the top of the priority pile, but I'm curious if you made any progress on this? Sorry for messing up the types in between the original analysis and the final merge to firebase-js-sdk, that definitely set things back

@SCasarotto
Copy link
Author

I haven't made any progress. We also are not longer using this part of the RNF package so its dropped even lower priority for me. I likely won't get back to this. At this point master looks fine and wouldn't have caused me an issue. Id probably just close this pr.

@mikehardy
Copy link
Collaborator

No worries - and I totally understand. I'll see what I can do via a scan-and-salvage on it is I do like the types to line up personally - thanks for the original effort I do appreciate it!

@mikehardy mikehardy added Workflow: Needs Review Pending feedback or review from a maintainer. and removed blocked: firebase-support Pending feedback or review from google support or response on official sdk repo issue. Workflow: Waiting for User Response Blocked waiting for user response. labels Apr 6, 2021
@mikehardy mikehardy self-assigned this Apr 6, 2021
mikehardy added a commit that referenced this pull request Apr 8, 2021
This builds on the work both upstream and in this repo of @SCasarotto
from #4550
@mikehardy
Copy link
Collaborator

#5141 carries this work into the repo, thanks @SCasarotto !

@mikehardy mikehardy closed this Apr 8, 2021
@SCasarotto
Copy link
Author

Very welcome. Glad I could contribute even if only a little.

mikehardy added a commit that referenced this pull request Apr 9, 2021
This builds on the work both upstream and in this repo of @SCasarotto
from #4550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: database Firebase Realtime Database tools: typings TypeScript / Flow Workflow: Needs Review Pending feedback or review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛] Type Definition Error in database Types
3 participants