First pass at auto generating sdk configs by maneesht · Pull Request #7833 · firebase/firebase-tools

maneesht · 2024-10-14T18:49:39Z

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

joehan

Some small stuff around typing & where helper functions live, but this is mostly LGTM!

joehan · 2024-10-21T18:29:53Z

      return appData;
    },
  );
+export async function sdkInit(appPlatform: AppPlatform, options: any) {


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.

Also, please use the Options type instead of any here too

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

joehan · 2024-10-21T18:47:31Z

+import { logError } from "../logError";

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


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

joehan · 2024-10-21T19:48:24Z

+          });
+        } 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.`,


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

joehan · 2024-10-21T19:54:22Z

-        appPlatform = appMetadata.platform;
+    }
+    const outputDir = path.dirname(outputPath!);
+    fs.mkdirpSync(outputDir);


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

joehan · 2024-10-21T19:54:54Z

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


If possible, use a stronger type than any here.

joehan · 2024-10-21T19:56:59Z

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


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

joehan · 2024-10-21T19:58:39Z

-    if (appPlatform === AppPlatform.WEB) {
-      fileInfo.sdkConfig = configData;
+    if (platform === AppPlatform.WEB) {
+      console.log(`


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

…rebase-tools into mtewani/auto-apps-first-pass

joehan

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

joehan · 2024-10-21T23:23:40Z

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


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

joehan · 2024-10-21T23:26:19Z

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


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)

joehan · 2024-10-21T23:29:59Z

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


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;

joehan · 2024-10-22T00:13:54Z

 import { logger } from "../logger";
-import { promptOnce } from "../prompt";
 import { Options } from "../options";
+import * as path from "path";


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

joehan · 2024-10-22T00:14:37Z

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


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

…irst-pass' into mtewani/auto-apps-first-pass

…irst-pass

… crashes

…irst-pass

joehan

LGTM after formatting, with 1 suggestion

joehan · 2025-02-06T22:05:58Z

+  out?: string | boolean;
+}
+
+function logUse(platform: AppPlatform, filePath: string) {


Suggested change

function logUse(platform: AppPlatform, filePath: string) {

function logUse(platform: AppPlatform, filePath: string): void {

joehan · 2025-02-06T22:27:55Z

    "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}'",


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)

joehan

Tiny things

* 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 <joehanley@google.com>

* 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 <joehanley@google.com> * 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 <maneesht@users.noreply.github.com> --------- Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com> * 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 <chalo@monogram.io> * 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 <joehanley@google.com> * 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 <rosalyntan@google.com> Co-authored-by: Maneesh Tewani <maneesht@users.noreply.github.com> Co-authored-by: Joe Hanley <joehanley@google.com> Co-authored-by: Google Open Source Bot <firebase-oss-bot@google.com> Co-authored-by: Mathusan Selvarajah <mathusans52@gmail.com> Co-authored-by: Philip Su <39933441+fivecar@users.noreply.github.com> Co-authored-by: Chalo Salvador <chalo@monogram.io> Co-authored-by: Daniel Lee <danielylee@google.com> Co-authored-by: Harold Shen <hlshen@google.com> Co-authored-by: Sarah Clark <seclark@nextquestion.net> Co-authored-by: annajowang <31288696+annajowang@users.noreply.github.com>

maneesht added 11 commits October 14, 2024 11:48

First pass at auto generating sdk configs

c72612c

Fixed formatting issues

f740de4

Removed extra command

0432624

Deleted unnecessary files

2eeff9c

Fixed more linting'

5e6bb73

Removed test assertion

c04efda

Fixed formatting

3b6022e

Updated erros

4ab3e9f

Misc

204f126

Updated platforms list

2bcb58c

Undid last changes

0e67680

joehan reviewed Oct 21, 2024

View reviewed changes

maneesht and others added 5 commits October 21, 2024 15:58

Addressed comments

022a189

Fixed client test

23bf76e

Driveby type fixing

4f9bdfd

Merge branch 'mtewani/auto-apps-first-pass' of github.com:firebase/fi…

d4200f5

…rebase-tools into mtewani/auto-apps-first-pass

missed a spot

e6fa76d

joehan approved these changes Oct 22, 2024

View reviewed changes

maneesht and others added 3 commits October 22, 2024 09:26

Fixed test

fc20178

Merge remote-tracking branch 'refs/remotes/public/mtewani/auto-apps-f…

1bee4da

…irst-pass' into mtewani/auto-apps-first-pass

Merge branch 'master' into mtewani/auto-apps-first-pass

45506fe

maneesht marked this pull request as ready for review October 22, 2024 18:35

maneesht added 8 commits February 3, 2025 16:20

Merge remote-tracking branch 'public/master' into mtewani/auto-apps-f…

bf0a1b3

…irst-pass

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

c3963be

… crashes

Added intelligent sensing where app should be

40d8397

Fixed formatting

db0ac18

Fixed lint

1327522

Fixed app dir

e95bdad

Misc

b6af1e6

Merge remote-tracking branch 'origin/master' into mtewani/auto-apps-f…

cb3a09e

…irst-pass

Wrote tests

f676ff8

joehan reviewed Feb 6, 2025

View reviewed changes

Comment thread package.json Outdated

maneesht added 8 commits February 6, 2025 13:43

Reverted apps sdkconfig changes

76e7691

Merge remote-tracking branch 'public/master' into mtewani/auto-apps-f…

67fe143

…irst-pass

Fixed formatting

e9c7280

Small changes

4f744df

Revert shrinkwrap changes

5c4af31

Updated test:management script

f8d1848

Fixed apps-sdkconfig boolean check

f3cd670

Fixed more boolean

45b3f97

joehan approved these changes Feb 6, 2025

View reviewed changes

maneesht added 2 commits February 6, 2025 14:10

Fixed formatting

a37000c

Added changelog

453ec56

joehan reviewed Feb 6, 2025

View reviewed changes

maneesht added 7 commits February 6, 2025 15:44

Added new options

09f2ba3

Removed unused var

f5248f1

Added experimental flag

5ab216f

Moved apps:init behind a flag

9cb95e6

Added apps:init command

a236f0f

Removed unnecessary experiments

92bc6d6

Fixed changelog

804e0a7

joehan reviewed Feb 12, 2025

View reviewed changes

Comment thread src/experiments.ts Outdated

Comment thread src/experiments.ts Outdated

Comment thread src/management/apps.spec.ts Outdated

maneesht and others added 2 commits February 12, 2025 12:59

Addressed comments

7f05bb3

Merge branch 'master' into mtewani/auto-apps-first-pass

3409f41

joehan enabled auto-merge (squash) February 12, 2025 21:04

joehan merged commit 8cc734b into master Feb 12, 2025

yorifuji mentioned this pull request Feb 14, 2025

fix: Support json format in Firebase options for web for firebase CLI >=13.31.0 invertase/flutterfire_cli#373

Merged

7 tasks

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

	function logUse(platform: AppPlatform, filePath: string) {
	function logUse(platform: AppPlatform, filePath: string): void {

Conversation

maneesht commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maneesht commented Oct 14, 2024 •

edited

Loading