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: vertexAI package support #8236

Open
wants to merge 105 commits into
base: main
Choose a base branch
from
Open

feat: vertexAI package support #8236

wants to merge 105 commits into from

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Jan 17, 2025

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:

Copy link

vercel bot commented Jan 17, 2025

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

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 3, 2025 5:12pm

@russellwheatley russellwheatley changed the title init feat: vertexAI package support Jan 17, 2025
@russellwheatley
Copy link
Member Author

russellwheatley commented Jan 30, 2025

@mikehardy - just to give you an update. I was able to write e2e tests for fetch and fetch stream.

I was able to mock vertexAI responses here: fa07a0d (#8236)

To make this work, I edited the RequestUrl to point to our emulators here: 8e8a2c6 (#8236),
I then updated the env var in the e2e test runner: d0afb73 (#8236)

I think this is fine but just wanted to see what you thought. I thought this would be better than just a stream/fetch request.

The thing that really borked was when I updated the types to use RNFB types rather than firebase-js-sdk types which are bundled with app (and sneakily allowed it to build/compile TS): https://github.com/invertase/react-native-firebase/blob/fix-jest-tests/packages/app/package.json#L60

Here is the commit: 0c9f687 (#8236)

I've basically reproduced the types from auth and app check package but only copied the relevant API we need to get a token from either package.

This created an additional problem. I needed to add this config to vertexai tsconfig.json so it knew about the app package (it's a peer dependency ):30b2b0e (#8236). But more importantly, to the root tsconfig.json as jest test runner would fail without it.

But this broke our TS validation as it now couldn't find firebase.storage(), firebase.auth() types on the FirebaseApp type. So I extended the tsconfig.json just for Jest: d378aee (#8236)

The last bits from my perspective are merging this: #8257

and figuring out why vertexai usage docs are broken on preview.

@russellwheatley
Copy link
Member Author

russellwheatley commented Jan 31, 2025

What do you think of this, @mikehardy? I update the types in this commit to use actual auth and app check types: 3514416 (#8236)

I had a look initially at using relative paths to auth and app check, but in the dist/ directory it kept the relative paths to auth and app check which naturally won't exist when published. So I used the paths property in tsconfig.json:

    "paths": {
      "@react-native-firebase/app": ["../app/lib"],
      "@react-native-firebase/auth": ["../auth/lib"],
      "@react-native-firebase/app-check": ["../app-check/lib"],
    }

which, for example, now generates the following types in dist/commonjs/lib/service.d.ts:

import { ReactNativeFirebase } from '@react-native-firebase/app';
import { VertexAI, VertexAIOptions } from './public-types';
import { FirebaseAuthTypes } from '@react-native-firebase/auth';
import { FirebaseAppCheckTypes } from '@react-native-firebase/app-check';
export declare class VertexAIService implements VertexAI {
    app: ReactNativeFirebase.FirebaseApp;
    options?: VertexAIOptions | undefined;
    auth: FirebaseAuthTypes.Module | null;
    appCheck: FirebaseAppCheckTypes.Module | null;
    location: string;
    constructor(app: ReactNativeFirebase.FirebaseApp, auth?: FirebaseAuthTypes.Module, appCheck?: FirebaseAppCheckTypes.Module, options?: VertexAIOptions | undefined);
}
//# sourceMappingURL=service.d.ts.map

If the user does pass in auth instance, they will naturally install auth dependency, and therefore import { FirebaseAuthTypes } from '@react-native-firebase/auth'; I believe will be able to reference the correct types from that import if that makes sense.

@mikehardy
Copy link
Collaborator

@russellwheatley interesting - seems fine?

I tested it by

  1. altering my make-demo script to only do @react-native-firebase/app (removing all other module install and references) and confirming it works, then
  2. adding the result of npm pack in the app (for the new common logger changes...) and vertexai packages of this PR to my demo and making sure vertexai worked, it still does, then
  3. adding app-check and auth packages and verifying getVertexAI with app-check and auth parameters worked, it does

So...this seems to work fine

Copy link
Collaborator

@MichaelVerdon MichaelVerdon left a comment

Choose a reason for hiding this comment

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

Cool!


export const PACKAGE_VERSION = version;

export const LANGUAGE_TAG = 'gl-js';
Copy link

Choose a reason for hiding this comment

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

Can you use another tag here? This is going to log all usage from this package as if it came from the JS SDK package. The full code for the headers is in request.ts if you want to customize it there too.

Copy link
Member

Choose a reason for hiding this comment

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

@hsubox76 any preference on your side what you'd like this to come through as? Happy to change

@russellwheatley russellwheatley added blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge and removed Needs Attention labels Feb 6, 2025
Copy link
Member Author

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

update global var

public requestOptions?: RequestOptions,
) {}
toString(): string {
const isTestEnvironment = process.env.RNFB_VERTEXAI_EMULATOR_URL;
Copy link
Member Author

Choose a reason for hiding this comment

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

Replace process.env with globalThis.

Comment on lines +11 to +13
packages/vertexai/__tests__/test-utils
packages/vertexai/dist
Copy link
Collaborator

Choose a reason for hiding this comment

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

the .eslintignore file has a non-trivial conflict from #8262 - the file is completely gone as eslint config changed completely across major versions. You add new ignores to the new .eslint.mjs file following pattern there

note the glob / regex style changed just a bit but the pattern is easy to follow

https://github.com/invertase/react-native-firebase/blob/main/eslint.config.mjs#L17-L39

@mikehardy
Copy link
Collaborator

made a specific note in relevant area about the eslint config conflict
the package.json conflict should be trivial to resolve, yarn.lock probably easiest to resolve by just removing it completely after resolving package.json conflict and committing result of fresh yarn.lock after merging in main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature request: Support for VertexAi
8 participants