-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(firestore): add VectorValue type and vector() API #8739
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
base: main
Are you sure you want to change the base?
Conversation
|
@Pelleking is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
|
The issue it resolves #8442 |
|
Hey @Pelleking 👋 ! Sorry this has sat, it's through no fault of your own, I've got quite a stack of PRs to review right now in this repo and this is definitely one of them. One thing I can do is approve the workflows though - in particular, you might like the output of the "patches" workflow that generates a set of patches compatible with patch-package, so you may continue to rely on the public packages we publish on npmjs.org, but apply the changes you need on top of them until this is merged |
38a52cd to
7af280b
Compare
|
@mikehardy Could you please rerun the workflows? |
|
Reviewer note - need to pull this along with withConverter PR support to check intersectionality - both of these are going to land, then after we can stage firestore for typescript-native conversion Re-running CI now and there have been a few CI fixes recently that may need to be rebased in here - in particular an android emulator hang when v36 of the emu came out a few days ago resolved with a version pin |
16d20b9 to
3475835
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some quick comments - all seem pretty easy, I'll try to push the suggested changes and get this merged quickly as we've got a lot of Firestore work queued up and it is all inter-related / will conflict
vector->Vector.fromJSONstatic to match firebase-js-sdk types-
Vector.fromJSONstill needs an implementation, but in factvectoris a correct JS API
-
- native code should rely on concrete types vs reflection, analysis shows it is non-breaking
- some I-think-unnecessary conditional definition checks are likely not needed
looks like native code mostly treats VectorValue as a FieldValuetests should be nested into the FieldValue e2e area- tests are currently attempting to operate on a doc ref that doesn't have permission
FieldValue parsing should be used I think in the serializer - not a new main type but a new FieldValue sub-type
.../reactnative/java/io/invertase/firebase/firestore/ReactNativeFirebaseFirestoreSerialize.java
Outdated
Show resolved
Hide resolved
needs a little nudge on JS API but not much so strange that this is ... sort of a FieldValue but is not really a FieldValue
3475835 to
3da6392
Compare
|
I have local e2e passing now after a bit of test alteration and some code change on native side This one was admittedly confusing for me - I haven't used the vector feature before and the typing is such that it is sort of a FieldValue (which threw me off) but is in the end not quite a FieldValue (because it isn't just a sentinel, they come back in document fetch as well). A strange-ish hybrid type. Anyway, serialization is working well now with just minor tweaks to make it reflexive vs concrete - just need to true up the JS API to match firebase-js-sdk |
- use preferred `vector()` method to instantiate/deserialize - avoid internals access and use toArray to serialize
…e area + modular types only - we are no longer adding namespaced-functionality - the public API surface area does not expose internal values or constructor - public API surface area does include fromJSON static method - supported usage is to create with `vector` or `fromJSON`, serialize w/toArray()
mikehardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- matches firebase-js-sdk modular API surface area and documentation now
- added types test entries
- passing all unit and e2e tests locally
should be good to go
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thank you so much @mikehardy for the help! |
|
Happy to help @Pelleking thanks for the original contribution and the patience |
feat(firestore): add VectorValue type and vector() API
Adds Firestore Vector field support to React Native Firebase:
VectorValueclass andvector(values?: number[])constructorand namespacedsurfaces (edit by @mikehardy - not adding new namespaced API surface area)Motivation
Brings RNFB Firestore to parity with Firebase SDKs that added Vector support:
JS SDK APIs (10.13): PR reference
iOS (11.10.0): release notes
Android (25.1.0): release notes
Fixes 🐛 [Firestore] Vector Type Support #8442
API
import { VectorValue, vector } from '@react-native-firebase/firestore'await setDoc(docRef, { embedding: vector([0.1, 0.2, 0.3]) })Namespaced:firebase.firestore.VectorValue,firebase.firestore.vector(values)Implementation
VectorValuewith validation,isEqual,toJSON, staticfromJSON,toArrayvectortype; wired intogenerateNativeDataandparseNativeDataVectorValueinweb/convert.jsVectorValuetype andvector()declaration; added toDocumentFieldTypeMinimum native versions for full functionality:
Older SDKs won’t crash (reflection guards), but vectors will not function.(edit by @mikehardy - min SDKs already present since prior to last RNFB breaking change, no problem)Usage Example
Tests
packages/firestore/__tests__/vector.test.tsTypes
packages/firestore/lib/index.d.tsto includeVectorValue,vector(), andDocumentFieldTypeunionChecklist
I read the Contributor Guide and followed the process outlined there for submitting PRs.
My change supports the following platforms;
My change includes tests;
packages/**/e2epackages/**/__tests__I have updated TypeScript types that are affected by my change.
This is a breaking change;
Notes
Bridges use reflection to avoid hard dependency on newer native symbols at build time.If project native SDKs are older than the minimums above, vector fields won’t be usable (graceful no-op).