-
Notifications
You must be signed in to change notification settings - Fork 942
Prepare for public preview release #8214
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
Conversation
🦋 Changeset detectedLatest commit: 57f6c0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1This report is too large (176,140 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
Size Analysis Report 1This report is too large (176,140 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.Test Logs |
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.
Approved with some nits and potential changes
packages/vertexai/package.json
Outdated
"description": "A template package for new firebase packages", | ||
"name": "@firebase/vertexai-preview", | ||
"version": "0.0.0", | ||
"description": "A Firebase SDK for VertexAI", |
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.
Suggestion: maybe we should we note that it's a preview version here in the description?
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.
Added, hopefully we catch this string when we go GA!
@@ -45,20 +45,26 @@ declare module '@firebase/component' { | |||
export function getVertexAI(app: FirebaseApp = getApp()): VertexAI { | |||
app = getModularInstance(app); | |||
// Dependencies | |||
const vertexProvider: Provider<'vertex'> = _getProvider(app, VERTEX_TYPE); | |||
const vertexProvider: Provider<'vertexAI'> = _getProvider(app, VERTEX_TYPE); |
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.
Could change Provider<`vertexAI`> to Provider<VERTEX_TYPE> here, I think.
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.
Sadly I cannot, Typescript can't turn an actual code string into a string that you use for typing.
@@ -74,6 +74,11 @@ export interface GenerateContentResponse { | |||
usageMetadata?: UsageMetadata; | |||
} | |||
|
|||
/** | |||
* Usage metadata about response. |
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.
nit a response
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.
Could even link to GenerateContentResponse
, will do that.
api-report
npm script so that it will be picked up by the docgen process (will generate docs in a separate PR)ch-vertex-feature
branch being merged intomaster
, so that's why it includes@firebase/app
.I did not rename the directory
packages/vertexai
because I didn't want this PR to have a 140 file diff. It should be fine, the dirname isn't published to NPM.