Implement Experiments feature #4994

inlined · 2022-09-19T20:54:09Z

This is a WIP. I still need to add the experiments command (which, ironically, until API council approves will be hidden behind a preview/experiment).

TODO: Add experiments:enable/disable/list/describe/clear commands (follow-up PR?)
TODO: Get representatives of each experiment to provide a shortDescription, verify if any should be public, and verify any assertEnabled calls.

inlined · 2022-09-19T20:55:32Z

src/deploy/functions/build.ts

@@ -281,7 +281,7 @@ export async function resolveBackend(
  nonInteractive?: boolean
 ): Promise<{ backend: backend.Backend; envs: Record<string, params.ParamValue> }> {
  let paramValues: Record<string, params.ParamValue> = {};
-  if (previews.functionsparams) {
+  if (experiments.isEnabled("functionsparams")) {


@Berlioz Does this still need to be hidden behind an experiment or do we think we are stable enough to roll forward? (If not, can we leave a comment on when this should be rolled forward?)

inlined · 2022-09-19T20:56:10Z

src/deploy/functions/prepare.ts

@@ -272,7 +272,7 @@ export async function prepare(
   * This must be called after `await validate.secretsAreValid`.
   */
  updateEndpointTargetedStatus(wantBackends, context.filters || []);
-  if (previews.skipdeployingnoopfunctions) {
+  if (experiments.isEnabled("skipdeployingnoopfunctions")) {


@TheIronDev When does this experiment go away?

src/deploy/index.ts

src/experiments.ts

inlined · 2022-09-19T21:30:00Z

A note to all people tagged for review on your experiment. If your experiment is going to have external users, you should add a log whenever executing experimental codepaths that mentions "experimental" in bold. E.g. "Deploying functions with experimental support for Python" or "Deploying hosting with experimental support for web frameworks"

src/handlePreviewToggles.ts

codecov-commenter · 2022-09-23T01:06:04Z

Codecov Report

Base: 55.88% // Head: 55.89% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (b0dfaa8) compared to base (20d3d7c).
Patch coverage: 47.69% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4994   +/-   ##
=======================================
  Coverage   55.88%   55.89%           
=======================================
  Files         305      305           
  Lines       20325    20351   +26     
  Branches     4094     4100    +6     
=======================================
+ Hits        11359    11375   +16     
- Misses       8011     8021   +10     
  Partials      955      955

Impacted Files	Coverage Δ
src/init/features/functions/index.ts	`82.66% <ø> (+1.89%)`	⬆️
src/emulator/controller.ts	`13.07% <10.00%> (-0.08%)`	⬇️
src/utils.ts	`66.19% <11.11%> (ø)`
src/deploy/index.ts	`29.57% <33.33%> (-0.43%)`	⬇️
src/deploy/functions/build.ts	`56.28% <50.00%> (ø)`
src/deploy/functions/prepare.ts	`30.05% <50.00%> (ø)`
src/experiments.ts	`58.06% <58.06%> (ø)`
src/deploy/functions/release/planner.ts	`87.40% <100.00%> (+0.28%)`	⬆️
src/deploy/functions/runtimes/index.ts	`54.16% <100.00%> (-1.84%)`	⬇️
src/emulator/downloadableEmulators.ts	`23.30% <100.00%> (ø)`
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

inlined · 2022-09-27T01:32:07Z

ping?

src/commands/experiments-clear.ts

bkendall · 2022-09-27T19:55:07Z

src/commands/experiments-clear.ts

+      logger.error(`Did you mean ${potentials[0]}?`);
+    } else if (potentials.length) {
+      logger.error(
+        `Did you mean ${potentials.slice(0, -1).join(",")} or ${potentials[potentials.length - 1]}?`


optionally:

Suggested change

`Did you mean ${potentials.slice(0, -1).join(",")} or ${potentials[potentials.length - 1]}?`

`Did you mean ${potentials.slice(0, -1).join(",")} or ${utils.last(potentials)}?`

Sure, but I also changed the signature of last to never return undefined since that only happened when someone passed a non-array which should never happen due to the signature of the method.

I'm an idiot and last can return undefined if the array is non-empty. Will fix

src/commands/experiments-describe.ts

src/commands/experiments-clear.ts

src/commands/experiments-disable.ts

src/commands/index.ts

src/deploy/index.ts

src/experiments.ts

src/handlePreviewToggles.ts

bkendall · 2022-09-28T22:12:08Z

src/commands/experiments-clear.ts

+      experiments.setEnabled(experiment, null);
+      experiments.flushToDisk();
+      logger.info(`Cleared preferences for experiment ${bold(experiment)}`);
+      return;


Should this throw instead?

Actually, Michael asked us to remove this feature for now.

bkendall · 2022-09-28T22:12:15Z

src/commands/experiments-describe.ts

+      } else if (potentials.length) {
+        logger.error(`Did you mean ${potentials.slice(0, -1).join(",")} or ${last(potentials)}?`);
+      }
+      return;


Should this throw instead?

bkendall · 2022-09-28T22:13:29Z

src/commands/experiments-list.ts

+    }
+    table.push(["n", name, exp.shortDescription]);
+  }
+  logger.info(table.toString());


One detail that can be punted on until later: for --json (machine readable) output, we could export the table as a JSON-ish object (just return an object in this method). Would that be easy enough to do?

I couldn't get this to work. You'll have to teach me how --json works with the commands library

Add experiments to the Firebase CLI. New commands: ``` firebase experiments:enable <experiment> firebase experiments:disable <experiment> firebase experiments:describe <experiment> firebase experiments:list ```

Checkpoint; wip

269cf66

inlined added the DO NOT MERGE label Sep 19, 2022

inlined requested a review from bkendall September 19, 2022 20:54

Add mandatory JSDocs

edc5fb7

inlined changed the title ~~Add official experiments support~~ WIP: Migrate to experiments library Sep 19, 2022

inlined added 2 commits September 19, 2022 14:08

Add missing file

fbc4800

Remove dead code

7fb879d

inlined commented Sep 19, 2022

View reviewed changes

src/experiments.ts Outdated Show resolved Hide resolved

bkendall reviewed Sep 19, 2022

View reviewed changes

src/handlePreviewToggles.ts Show resolved Hide resolved

Add experiments commands

a5cc083

inlined removed the DO NOT MERGE label Sep 22, 2022

inlined added 4 commits September 22, 2022 11:55

Add missing files

e51cc64

Merge remote-tracking branch 'origin/master' into inlined.experiments

8de1c7f

Remove dead experiments

e7445c0

Update docuentation strings

7038e3c

inlined changed the title ~~WIP: Migrate to experiments library~~ Implement Experiments feature Sep 22, 2022

inlined requested a review from bkendall September 23, 2022 00:00

Fix case of no previews config (new machines)

aa77661

Merge branch 'master' into inlined.experiments

363dc93

bkendall requested changes Sep 27, 2022

View reviewed changes

bkendall self-assigned this Sep 27, 2022

PR feedback

f19c209

inlined requested a review from bkendall September 28, 2022 16:45

inlined added 3 commits September 28, 2022 10:20

Fix type conversion issue

a069947

Merge + changes

4984825

Merge remote-tracking branch 'origin/master' into inlined.experiments

8a461f8

bkendall approved these changes Sep 28, 2022

View reviewed changes

inlined added 2 commits September 28, 2022 16:33

PR feedback

7808be8

Merge remote-tracking branch 'origin/master' into inlined.experiments

ede6af4

inlined enabled auto-merge (squash) September 28, 2022 23:34

inlined and others added 2 commits September 29, 2022 11:00

Fix linter error

05ac750

Merge branch 'master' into inlined.experiments

b0dfaa8

inlined merged commit c3ec450 into master Sep 29, 2022

inlined deleted the inlined.experiments branch September 29, 2022 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Experiments feature #4994

Implement Experiments feature #4994

inlined commented Sep 19, 2022

inlined Sep 19, 2022

inlined Sep 19, 2022

inlined commented Sep 19, 2022

codecov-commenter commented Sep 23, 2022 •

edited

inlined commented Sep 27, 2022

bkendall Sep 27, 2022

inlined Sep 28, 2022

inlined Sep 30, 2022

bkendall Sep 28, 2022

inlined Sep 28, 2022

bkendall Sep 28, 2022

inlined Sep 28, 2022

bkendall Sep 28, 2022

inlined Sep 28, 2022

	`Did you mean ${potentials.slice(0, -1).join(",")} or ${potentials[potentials.length - 1]}?`
	`Did you mean ${potentials.slice(0, -1).join(",")} or ${utils.last(potentials)}?`

Implement Experiments feature #4994

Implement Experiments feature #4994

Conversation

inlined commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inlined commented Sep 19, 2022

codecov-commenter commented Sep 23, 2022 • edited

Codecov Report

inlined commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 23, 2022 •

edited