Skip to content

First pass at auto generating sdk configs #7833

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

Merged
merged 47 commits into from
Feb 12, 2025
Merged

Conversation

maneesht
Copy link
Contributor

@maneesht maneesht commented Oct 14, 2024

Fixed:

  • Return type. Originally the object returned by apps-sdkconfig was typed as any, and didn't match the correct AppConfigurationData type. I returned the right object instead.
  • When file name is not specified via out and the command attempted to get the default file name, the terminal would output undefined and error out. Now the correct output file is specified and used.
  • Implemented relativeTo when prompting for directories
➜  dataconnect git:(mtewani/idx-updates) node ~/source/firebase-tools/lib/bin/firebase.js apps:init -o false

Error: Cannot autoconfigure an app because the experiment appsinit is not enabled. To enable appsinit run firebase experiments:enable appsinit

Having trouble? Try firebase [command] --help
➜  app git:(mtewani/idx-updates) node ~/source/firebase-tools/lib/bin/firebase.js apps:init
✔  Detected WEB app in directory /Users/mtewani/source/quickstart-js/dataconnect/app
✔ Downloading configuration data for your Firebase WEB app
App configuration is written in firebase-js-config.json

How to use your JS SDK Config:
ES Module:
import { initializeApp } from 'firebase/app';
import json from './firebase-js-config.json';
initializeApp(json); // or copy and paste the config directly from the json file here
// CommonJS Module:
const { initializeApp } = require('firebase/app');
const json = require('./firebase-js-config.json');
initializeApp(json); // or copy and paste the config directly from the json file here

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 small stuff around typing & where helper functions live, but this is mostly LGTM!

logPostAppCreationInformation(appData, appPlatform);
return appData;
},
);
export async function sdkInit(appPlatform: AppPlatform, options: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to move this to a separate file - I like to keep these command files as lean as possible and only export the command itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please use the Options type instead of any 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.

Should we move everything into a new file? Also, we should put that info somewhere, I think other commands have other functions besides just the Command


function checkForApps(apps: AppMetadata[], appPlatform: AppPlatform): void {
export function getSdkOutputPath(appDir: string, platform: Platform): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above - prefer to put this in a separate file than the command

});
} else if (targetPlatform === Platform.FLUTTER) {
logError(
`Flutter is not supported by apps:sdkconfig. Please install the flutterfire CLI and run "flutterfire configure" to set up firebase for your flutter app.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Include a link to the flutterfire getting started docs?

appPlatform = appMetadata.platform;
}
const outputDir = path.dirname(outputPath!);
fs.mkdirpSync(outputDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about mkdirp - we probably ought to use it in quite a few other places

}
const outputDir = path.dirname(outputPath!);
fs.mkdirpSync(outputDir);
let sdkConfig: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, use a stronger type than any here.

choices: platforms,
});
} else if (targetPlatform === Platform.FLUTTER) {
logError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking this behavior - what happens after this logging? Should we error out/return instead?

if (appPlatform === AppPlatform.WEB) {
fileInfo.sdkConfig = configData;
if (platform === AppPlatform.WEB) {
console.log(`
Copy link
Contributor

Choose a reason for hiding this comment

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

Use logger.log() or logger.info() instead - console.log circumvents some verbosity/VSCode specific log handling we have in place.

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.

Made a driveby type cleanup commit, and a few other nitty things, but LGTM!

sdkConfig = await getSdkConfig(options, getAppPlatform(platform), appId);
} catch (e) {
if ((e as Error).message.includes("associated with this Firebase project")) {
await sdkInit(platform as unknown as AppPlatform, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this type washing is now unnecessary (since you strongly typed platform in all locations afaict)

const skipWrite = options.out === false;
const config = options.config;
const appDir = process.cwd();
if (!platform) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ever hit? I don't think AppPlatform.PLATFORM_UNSPECIFIED is falsey (since its a string enum). Feels like this should maybe be if (platform === AppPlatform.PLATFORM_UNSPECIFIED)

appId = "",
options: AppsSdkConfigOptions,
): Promise<AppConfig> => {
let outputPath: string | undefined = options.out === true ? "" : (options.out as string);
Copy link
Contributor

Choose a reason for hiding this comment

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

You called it out in the comment above, but this is extremely code smelly. Consider something like:

Suggested change
let outputPath: string | undefined = options.out === true ? "" : (options.out as string);
let outputPath: string | undefined = (options.out || "" ) as string;

import { Options } from "../options";
import * as path from "path";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: prefer grouping this with the other external imports at the top of the file.

throw err;
}
}

export const command = new Command("apps:create [platform] [displayName]")
.description(
"create a new Firebase app. [platform] can be IOS, ANDROID or WEB (case insensitive).",
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, could you add a type for options on this command as well?

@maneesht maneesht marked this pull request as ready for review October 22, 2024 18:35
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.

LGTM after formatting, with 1 suggestion

out?: string | boolean;
}

function logUse(platform: AppPlatform, filePath: string) {
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
function logUse(platform: AppPlatform, filePath: string) {
function logUse(platform: AppPlatform, filePath: string): void {

@@ -28,6 +28,7 @@
"test": "npm run lint:quiet && npm run test:compile && npm run mocha",
"test:client-integration": "bash ./scripts/client-integration-tests/run.sh",
"test:compile": "tsc --project tsconfig.compile.json",
"test:management": "npm run lint:quiet && npm run test:compile && nyc mocha 'src/management/*.spec.{ts,js}'",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably unecessary since these unit tests are covered by npm run mocha. All these other e2e tests have special commands because they use code from ./scripts and do things that are not appropriate for unit tests (ie calls to prod with credentials)

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.

Tiny things

@joehan joehan enabled auto-merge (squash) February 12, 2025 21:04
@joehan joehan merged commit 8cc734b into master Feb 12, 2025
51 of 56 checks passed
tammam-g pushed a commit that referenced this pull request Mar 10, 2025
* First pass at auto generating sdk configs

* Fixed formatting issues

* Removed extra command

* Deleted unnecessary files

* Fixed more linting'

* Removed test assertion

* Fixed formatting

* Updated erros

* Misc

* Updated platforms list

* Undid last changes

* Addressed comments

* Fixed client test

* Driveby type fixing

* missed a spot

* Fixed test

* Fix issue where if a user passes in an empty 'out' parameter, the CLI crashes

* Added intelligent sensing where app should be

* Fixed formatting

* Fixed lint

* Fixed app dir

* Misc

* Wrote tests

* Reverted apps sdkconfig changes

* Fixed formatting

* Small changes

* Revert shrinkwrap changes

* Updated test:management script

* Fixed apps-sdkconfig boolean check

* Fixed more boolean

* Fixed formatting

* Added changelog

* Added new options

* Removed unused var

* Added experimental flag

* Moved apps:init behind a flag

* Added apps:init command

* Removed unnecessary experiments

* Addressed comments

---------

Co-authored-by: Joe Hanley <[email protected]>
tammam-g added a commit that referenced this pull request Mar 11, 2025
* Helper functions and basic setup for dataconnect:sql:setup

* Refactor setupIamUsers for better composability

* FDC MVP brownfield and greenfield to brownfield schema setup

* Add required logic inside schemaMigration for handling brownfield

* Cleanup and fix bugs in brownfield setup

* Use firebasesuperuser instead of cloudsqlsuper user for brownfield migration success

* Add default permissions for brownfield

* Fix lint/format

* Refactor to allow setup reruns

* Fix small things and address comments

* Fix bug in role grants

* Add logging that database setup completed

* Make grant command not go through setup if roles can be granted in brownfield

* bug fix from changing the getting schema owner command

* Simplify getSchemaMetaData in permissions.ts

* Fix log statement

* Split permissions.ts into front facing permissions_setup.ts and keep backend permissions there

* No need to ask user if they want to rerun greenfield setup

* Make setupSQLPermissions return a setup status instead of a boolean

* Change an if statment to switch statement

* Keep upserting new user in grant command

* Bump FDC local toolkit to v1.8.0. (#8210)

* Bump FDC local toolkit to v1.8.0.

* Update changelog.

* First pass at auto generating sdk configs (#7833)

* First pass at auto generating sdk configs

* Fixed formatting issues

* Removed extra command

* Deleted unnecessary files

* Fixed more linting'

* Removed test assertion

* Fixed formatting

* Updated erros

* Misc

* Updated platforms list

* Undid last changes

* Addressed comments

* Fixed client test

* Driveby type fixing

* missed a spot

* Fixed test

* Fix issue where if a user passes in an empty 'out' parameter, the CLI crashes

* Added intelligent sensing where app should be

* Fixed formatting

* Fixed lint

* Fixed app dir

* Misc

* Wrote tests

* Reverted apps sdkconfig changes

* Fixed formatting

* Small changes

* Revert shrinkwrap changes

* Updated test:management script

* Fixed apps-sdkconfig boolean check

* Fixed more boolean

* Fixed formatting

* Added changelog

* Added new options

* Removed unused var

* Added experimental flag

* Moved apps:init behind a flag

* Added apps:init command

* Removed unnecessary experiments

* Addressed comments

---------

Co-authored-by: Joe Hanley <[email protected]>

* 13.31.0

* [firebase-release] Removed change log and reset repo after 13.31.0 release

* FDC Emulator Update v1.8.1(#8216)

* 13.31.1

* [firebase-release] Removed change log and reset repo after 13.31.1 release

* Update formatting of connector evolution and insecure operation issues. (#8204)

* Format INTERACTIVE_ACK issues as table as well and add extra "type" column to table.

* Update warning and prompt wording to reflect insecure operations as well as connector evolution issues.

* Wording.

* Sort issues in table by category and some formatting fixes.

* Use correct import path for data connect emulator (#8220)

* Use correct import path for data connect emulator

* Actually fix it this time

* fix the thing i broke and format

* Update src/emulator/dataconnect/pgliteServer.ts

Co-authored-by: Maneesh Tewani <[email protected]>

---------

Co-authored-by: Maneesh Tewani <[email protected]>

* Don't surface insecure operations errors in VSCode. (#8215)

* Don't surface connector evolution or insecure operation issues in VSCode.'

* Fix

* Filter by "INSECURE" substring rather than warningLevel.

* Add path information to formatted GraphqlError. (#8228)

* App Hosting Emulator bug - apphosting emulator info is not complete when env vars for emulators are set (#8231)

* fix bug where apphosting emulator info is not complete when env vars for other emulators are set

* add proper fix and test

* fix

* remove async from non-async test func

* address comments

* Bump FDC local toolkit to v1.8.2. (#8232)

* Bump FDC local toolkit to v1.8.2.

* Update changelog.

* 13.31.2

* [firebase-release] Removed change log and reset repo after 13.31.2 release

* fix: #8168 - enforce webframeworks only when needed (#8169)

* fix: #8168 - enforce webframeworks only when needed

In deployments where `--only hosting:boo` is used, enforce webframeworks
enablement only when the target actually uses webframeworks.

* remove console

* add changelog, add tests

* Add matchesHostingTarget to improve readability

---------

Co-authored-by: Chalo Salvador <[email protected]>

* Added env var to magically import data connect service from console (#8237)

* Added env var to magically import data connect service from console

* actually check location too

* formats

* Formats

* Add initial delay when loading python functions (#8239)

* Improve robustness of function discovery for python

Anecdotally, python function discovery is flakey. We propose 2 change in
this PR:

1. For python discovery, add a small initial delay for python's admin
server to boot.

2. Add a request timeout to retry call to retrieve trigger information.
Previously, the default timeout would've been set to OS-level TCP
timeout, which in my laptop was between 20~30s.

* Add changelog.

* Remove per-req timeout to accomodate loading large/slow main.py.

* Update changelog.

* Revert timeout bump.

* Update vscode to 0.13.1 (#8236)

* update vscode to 0.13.1

* remove changelog line

* Propagate overrides (#8253)

* Apply ajv and ajv-formats overrides to shrinkwrap

* Apply whatwg-url override to shrinkwrap

* npm i to stabilize shrinkwrap

---------

Co-authored-by: Joe Hanley <[email protected]>

* Print warning about --location removal from apphosting commands. (#8229)

`--location` will be removed from apphosting commands in the next major CLI release. Before then, the CLI will print a warning about this removal whenever `--location` is used.

* Fix issue where apps:init breaks on app creation (#8258)

* Rename MetaData to Metadata

* Change setup to set up in firebase error

* Improve logger message

* Fix bugs in brownfield setup status checks

* fix lint issues

---------

Co-authored-by: Rosalyn Tan <[email protected]>
Co-authored-by: Maneesh Tewani <[email protected]>
Co-authored-by: Joe Hanley <[email protected]>
Co-authored-by: Google Open Source Bot <[email protected]>
Co-authored-by: Mathusan Selvarajah <[email protected]>
Co-authored-by: Philip Su <[email protected]>
Co-authored-by: Chalo Salvador <[email protected]>
Co-authored-by: Daniel Lee <[email protected]>
Co-authored-by: Harold Shen <[email protected]>
Co-authored-by: Sarah Clark <[email protected]>
Co-authored-by: annajowang <[email protected]>
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