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

Add functions for creating Firestore for a given database #482

Closed
wants to merge 5 commits into from

Conversation

AKJAW
Copy link

@AKJAW AKJAW commented Mar 28, 2024

The current top level functions don't allow creating Firestore for specific databases in the same Google Cloud project
Screenshot 2024-03-28 at 07 47 06

It is possible by having some work-around using expect actual but it's a big hassle and frakly I wasn't able to make pods cooperate with me
Screenshot 2024-03-28 at 07 45 26
Screenshot 2024-03-28 at 07 45 40

This PR "tries" to introduce those functions however I'm not sure about the JS implementations since I wasn't able to get the Repository to synchronize.

Comment on lines +76 to +78
external fun getFirestore(app: FirebaseApp? = definedExternally, database: String = definedExternally): Firestore

external fun getFirestore(database: String = definedExternally): Firestore
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is needed / allowed

@AKJAW AKJAW marked this pull request as ready for review March 28, 2024 06:49
@nbransby
Copy link
Member

Thanks @AKJAW, could you add a unit test for it? Then we can confirm if it works in JS

@AKJAW
Copy link
Author

AKJAW commented Mar 28, 2024

Thanks @AKJAW, could you add a unit test for it? Then we can confirm if it works in JS

The problem is that I cannot get the repository to synchronize properly, but if you could point me to some existing tests I can try to create something

@nbransby
Copy link
Member

@AKJAW
Copy link
Author

AKJAW commented Mar 28, 2024

Existing tests are here: https://github.com/GitLiveApp/firebase-kotlin-sdk/blob/master/firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt

Are there any existing tests for initialization? All I see that it is used as the set-up for others tests. Do you just want me to create a test which uses these functions and verifies that there is no exception?

@nbransby
Copy link
Member

Do you just want me to create a test which uses these functions and verifies that there is no exception?

sounds good

@AKJAW AKJAW force-pushed the firestore-database-name branch from 67ef08c to 3b050c4 Compare March 29, 2024 14:15
@AKJAW
Copy link
Author

AKJAW commented Mar 29, 2024

Do you just want me to create a test which uses these functions and verifies that there is no exception?

sounds good

i've added the tests, however I'm not sure if the one that doesn't specify app will be able to pass, since it probably won't be able to retrieve the app from a global context

@nbransby
Copy link
Member

nbransby commented Apr 1, 2024

Yes the JS tests are failing with

e: file:///home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt:64:33 Overload resolution ambiguity: 

> Task :firebase-firestore:compileKotlinJs FAILED
public external fun getFirestore(app: FirebaseApp? = ...): Firestore defined in dev.gitlive.firebase.firestore.externals in file firestore.kt
public external fun getFirestore(app: FirebaseApp? = ..., database: String = ...): Firestore defined in dev.gitlive.firebase.firestore.externals in file firestore.kt
public external fun getFirestore(database: String = ...): Firestore defined in dev.gitlive.firebase.firestore.externals in file firestore.kt

@nbransby
Copy link
Member

nbransby commented Apr 1, 2024

The android tests are also failing but the test report for firestore appear to be missing, im not sure why, best to try running them locally

@AKJAW
Copy link
Author

AKJAW commented Apr 14, 2024

The android tests are also failing but the test report for firestore appear to be missing, im not sure why, best to try running them locally

Hello, we decided to not use this library, and I still wasn't able to get the project running locally, so I guess I'll close this pull request / convert it to a feature request for the future

@AKJAW AKJAW closed this Apr 14, 2024
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.

2 participants