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 firebase ext:sdk:install command #7581

Merged
merged 24 commits into from
Sep 21, 2024
Merged

Add firebase ext:sdk:install command #7581

merged 24 commits into from
Sep 21, 2024

Conversation

ifielker
Copy link
Contributor

Add firebase ext:sdk:install command

src/extensions/runtimes/node.ts Fixed Show fixed Hide fixed
src/extensions/runtimes/node.ts Fixed Show fixed Hide fixed
src/extensions/runtimes/node.ts Fixed Show fixed Hide fixed
@ifielker ifielker requested a review from joehan September 18, 2024 21:22
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

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

Some style nits, but LGTM overall!

CHANGELOG.md Outdated
@@ -5,3 +5,4 @@
- Improved handling of Spark projects in `firebase init dataconnect`. (#7666)
- Updated Firebase Data Connect local toolkit version to v1.3.7, which adds support for `v1beta` gRPC APIs and the `OrderDirection` enum in Swift, and makes transactional queries and mutations opt-in with the `@transaction` directive. (#7679)
- Add dataconnect SQL grant command `firebase dataconnect:sql:grant -R <role> -E email`. (#7656)
- Added new command ext:sdk:install to allow you to configure extensions in a functions codebase. (#7581)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added new command ext:sdk:install to allow you to configure extensions in a functions codebase. (#7581)
- Added new command `firebase ext:sdk:install` to allow you to configure extensions in a functions codebase. (#7581)

Nit: If there are docs, it would be good to link them here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -121,5 +122,10 @@ export const command = new Command("ext:info <extensionName>")
`to install this extension, type ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`to install this extension, type ` +
`to install this extension, run ` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -121,5 +122,10 @@ export const command = new Command("ext:info <extensionName>")
`to install this extension, type ` +
clc.bold(`firebase ext:install ${extensionName} --project=YOUR_PROJECT`),
);
utils.logLabeledBullet(
logPrefix,
`to install an autogenerated SDK for this extension into your functions codebase, type ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`to install an autogenerated SDK for this extension into your functions codebase, type ` +
`to install an autogenerated SDK for this extension into your functions codebase, run ` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,111 @@
import * as clc from "colorette";
import { checkMinRequiredVersion } from "../checkMinRequiredVersion";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { checkMinRequiredVersion } from "../checkMinRequiredVersion";
import { checkMinRequiredVersion } from "../checkMinRequiredVersion";

Nit: We usually put a newline between external and internal imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

export async function haveDynamic(projectId: string): Promise<DeploymentInstanceSpec[]> {
let instances = await extensionsApi.listInstances(projectId);
// Only include instances created by SDK
instances = instances.filter((i) => i.labels?.createdBy === "SDK");
Copy link
Contributor

Choose a reason for hiding this comment

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

Optonal style nit: I like chaining these without setting to a variable - ie:
return (await extensionsApi.listInstances(projectId)).filter(...).map(...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// Make sure the directories exist
await fs.promises
.mkdir(path.dirname(filePath), { recursive: true })
.then(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This .then() should be unnecessary. Pretty sure this code works identically if you just delete it and replace the .catch with a standard try/catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ifielker ifielker merged commit e0089d6 into master Sep 21, 2024
41 checks passed
@ifielker ifielker deleted the if-extsdk-2 branch September 21, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants