Skip to content

Conversation

@ifielker
Copy link
Contributor

Add firebase ext:sdk:install command

@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
- 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

logger.info(await marked(lines.join("\n")));
utils.logLabeledBullet(
logPrefix,
`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

);
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants