Skip to content

utilize updated values from settings config #8882

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

utilize updated values from settings config #8882

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a60c49
Change instances of settings config to be fresh values
hlshen Jul 22, 2025
95dbd39
dedupe getsettings
hlshen Jul 22, 2025
cde6249
changelog
hlshen Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions firebase-vscode/src/analytics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,5 @@ export function getAnalyticsContext(context: vscode.ExtensionContext) {
}

function addFirebaseBinaryMetadata(data?: Record<string, any> | undefined) {
const settings = getSettings();
return { ...data, binary_kind: settings.firebaseBinaryKind };
return { ...data, binary_kind: getSettings().firebaseBinaryKind };
}
3 changes: 1 addition & 2 deletions firebase-vscode/src/core/emulators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@

// Subscription to trigger emulator exports when button is clicked.
this.subscriptions.push(broker.on("runEmulatorsExport", () => {
vscode.commands.executeCommand("firebase.emulators.exportData")

Check warning on line 45 in firebase-vscode/src/core/emulators.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing semicolon

Check warning on line 45 in firebase-vscode/src/core/emulators.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing semicolon
}));
}

Expand Down Expand Up @@ -168,8 +168,7 @@
}

async exportEmulatorData(): Promise<void> {
const settings = getSettings();
const exportDir = settings.exportPath;
const exportDir = getSettings().exportPath;
const hubClient = this.getHubClient();
if (hubClient) {
// TODO: Make exportDir configurable
Expand Down
11 changes: 5 additions & 6 deletions firebase-vscode/src/core/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export async function registerCore(
context: ExtensionContext,
analyticsLogger: AnalyticsLogger,
): Promise<[EmulatorsController, vscode.Disposable]> {
const settings = getSettings();

// Wrap the runTerminalTask function to allow for e2e testing.
const initSpy = createE2eMockable(
Expand All @@ -32,11 +31,11 @@ export async function registerCore(
async () => {},
);

if (settings.npmPath) {
process.env.PATH += `:${settings.npmPath}`;
if (getSettings().npmPath) {
process.env.PATH += `:${getSettings().npmPath}`;
}

if (settings.useFrameworks) {
if (getSettings().useFrameworks) {
setEnabled("webframeworks", true);
}

Expand Down Expand Up @@ -67,8 +66,8 @@ export async function registerCore(
return;
}
const initCommand = currentProjectId.value
? `${settings.firebasePath} init dataconnect --project ${currentProjectId.value}`
: `${settings.firebasePath} init dataconnect`;
? `${getSettings().firebasePath} init dataconnect --project ${currentProjectId.value}`
: `${getSettings().firebasePath} init dataconnect`;

initSpy.call("firebase init", initCommand, { focus: true });
});
Expand Down
6 changes: 2 additions & 4 deletions firebase-vscode/src/data-connect/deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ export function registerFdcDeploy(
broker: ExtensionBrokerImpl,
analyticsLogger: AnalyticsLogger,
): vscode.Disposable {
const settings = getSettings();

const deploySpy = createE2eMockable(
async (...args: Parameters<typeof runCommand>) => {
// Have the "deploy" return "void" for easier mocking (no return value when spied).
Expand All @@ -43,7 +41,7 @@ export function registerFdcDeploy(

const deployAllCmd = vscode.commands.registerCommand("fdc.deploy-all", () => {
analyticsLogger.logger.logUsage(DATA_CONNECT_EVENT_NAME.DEPLOY_ALL);
deploySpy.call(`${settings.firebasePath} deploy --only dataconnect`);
deploySpy.call(`${getSettings().firebasePath} deploy --only dataconnect`);
});

const deployCmd = vscode.commands.registerCommand("fdc.deploy", async () => {
Expand All @@ -65,7 +63,7 @@ export function registerFdcDeploy(
}

deploySpy.call(
`${settings.firebasePath} ${createDeployOnlyCommand(serviceConnectorMap)}`,
`${getSettings().firebasePath} ${createDeployOnlyCommand(serviceConnectorMap)}`,
); // run from terminal
});

Expand Down
4 changes: 1 addition & 3 deletions firebase-vscode/src/data-connect/sdk-generation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
broker: ExtensionBrokerImpl,
analyticsLogger: AnalyticsLogger,
): vscode.Disposable {
const settings = getSettings();

// For testing purposes.
const selectFolderSpy = createE2eMockable(
async () => {
Expand All @@ -40,10 +38,10 @@
(args: { appFolder: string }) => {
analyticsLogger.logger.logUsage(DATA_CONNECT_EVENT_NAME.INIT_SDK_CLI);
// Lets do it from the right directory
const e: Record<string, string> = {}

Check warning on line 41 in firebase-vscode/src/data-connect/sdk-generation.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing semicolon

Check warning on line 41 in firebase-vscode/src/data-connect/sdk-generation.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing semicolon
e[FDC_APP_FOLDER] = args.appFolder;
setTerminalEnvVars(e);
runCommand(`${settings.firebasePath} init dataconnect:sdk`);
runCommand(`${getSettings().firebasePath} init dataconnect:sdk`);
},
);

Expand Down
15 changes: 6 additions & 9 deletions firebase-vscode/src/data-connect/terminal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@ export function setTerminalEnvVars(env: Record<string, string>) {
}

export function runCommand(command: string) {
const settings = getSettings();
setTerminalEnvVars(settings.extraEnv ?? {});
setTerminalEnvVars(getSettings().extraEnv ?? {});
const terminalOptions: TerminalOptions = {
name: "Data Connect Terminal",
env: environmentVariables,
Expand All @@ -31,7 +30,7 @@ export function runCommand(command: string) {
if (currentProjectId.value) {
command = `${command} --project ${currentProjectId.value}`;
}
if (settings.debug) {
if (getSettings().debug) {
command = `${command} --debug`;
}
terminal.sendText(command);
Expand All @@ -42,8 +41,7 @@ export function runTerminalTask(
command: string,
presentationOptions: vscode.TaskPresentationOptions = { focus: true },
): Promise<string> {
const settings = getSettings();
setTerminalEnvVars(settings.extraEnv ?? {});
setTerminalEnvVars(getSettings().extraEnv ?? {});
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Similar to runCommand, the previous implementation of runTerminalTask was more efficient. This change introduces a performance regression by calling getSettings() twice (here and on line 67). Please cache the result of getSettings() in a local variable at the top of the function.

export function runTerminalTask(
  taskName: string,
  command: string,
  presentationOptions: vscode.TaskPresentationOptions = { focus: true },
): Promise<string> {
  const settings = getSettings();
  setTerminalEnvVars(settings.extraEnv ?? {});

const type = "firebase-" + Date.now();
return new Promise(async (resolve, reject) => {
vscode.tasks.onDidEndTaskProcess(async (e) => {
Expand All @@ -66,7 +64,7 @@ export function runTerminalTask(
vscode.TaskScope.Workspace,
taskName,
"firebase",
new vscode.ShellExecution(`${command}${settings.debug ? " --debug" : ""}`, executionOptions),
new vscode.ShellExecution(`${command}${getSettings().debug ? " --debug" : ""}`, executionOptions),
);
task.presentationOptions = presentationOptions;
await vscode.tasks.executeTask(task);
Expand All @@ -77,21 +75,20 @@ export function registerTerminalTasks(
broker: ExtensionBrokerImpl,
analyticsLogger: AnalyticsLogger,
): Disposable {
const settings = getSettings();

const loginTaskBroker = broker.on("executeLogin", () => {
analyticsLogger.logger.logUsage(DATA_CONNECT_EVENT_NAME.IDX_LOGIN);
runTerminalTask(
"firebase login",
`${settings.firebasePath} login --no-localhost`,
`${getSettings().firebasePath} login --no-localhost`,
).then(() => {
checkLogin();
});
});

const startEmulatorsTask = () => {
analyticsLogger.logger.logUsage(DATA_CONNECT_EVENT_NAME.START_EMULATORS);

const settings = getSettings();
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

It's redundant to call getSettings() again here, since it's already called on line 91. You can remove this call and use the settings variable that's already defined.

    let cmd = `${settings.firebasePath} emulators:start --project ${currentProjectId.value}`;

let cmd = `${settings.firebasePath} emulators:start --project ${currentProjectId.value}`;

if (settings.importPath) {
Expand Down
3 changes: 1 addition & 2 deletions firebase-vscode/src/data-connect/toolkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export class DataConnectToolkit implements vscode.Disposable {
// special function to start FDC emulator with special flags & port
async startFDCToolkit(configDir: string, config: Config, RC: RC) {
const port = await findOpenPort(DEFAULT_PORT);
const settings = getSettings();

const toolkitArgs: DataConnectEmulatorArgs = {
projectId: "toolkit",
Expand All @@ -48,7 +47,7 @@ export class DataConnectToolkit implements vscode.Disposable {
autoconnectToPostgres: false,
enable_output_generated_sdk: true,
enable_output_schema_extensions: true,
extraEnv: settings.extraEnv,
extraEnv: getSettings().extraEnv,
};
pluginLogger.info(`Starting Data Connect toolkit (version ${DataConnectToolkitController.getVersion()}) on port ${port}`);
return DataConnectToolkitController.start(toolkitArgs);
Expand Down
3 changes: 1 addition & 2 deletions firebase-vscode/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ export async function activate(context: vscode.ExtensionContext) {


await setupFirebasePath(analyticsLogger);
const settings = getSettings();
logSetup();
pluginLogger.debug("Activating Firebase extension.");

Expand All @@ -42,7 +41,7 @@ export async function activate(context: vscode.ExtensionContext) {
const authService = new AuthService(broker);

// show IDX data collection notice
if (settings.shouldShowIdxMetricNotice && env.value.isMonospace) {
if (getSettings().shouldShowIdxMetricNotice && env.value.isMonospace) {
// don't await/block on this
vscode.window.showInformationMessage(IDX_METRIC_NOTICE, "Ok").then(() => {
updateIdxSetting(false); // don't show message again
Expand Down
Loading