Insert Expression and Field types into GlobalOptions and inheriting types #1171

Berlioz · 2022-07-19T08:50:32Z

By changing the types we allow inside the GlobalOptions parameter of the various CF3 setup functions, we automatically plumb them through to the ManifestSpec. Derived from #1168, which was busted enough I decided to just make a new branch.

So far, this PR still does not include:

~~the trigger type specific parameterized fields~~
full adjustments to the comments for the changed fields
~~any tests whatsoever~~ but it seems like we're going to rely more on integration tests here
sanity checking on whether provided strings are actually CEL (but shouldn't we just validate that in the CLI?)
but I wanted to see whether I was totally barking up the wrong tree first.

src/v2/expressions.ts

taeold · 2022-07-22T18:38:54Z

spec/v2/providers/tasks.spec.ts

+ () => {}
+ );
+
+ expect(result.__trigger).to.deep.equal({


I wouldn't worry about __trigger annotation for v2.

taeold · 2022-07-22T18:39:59Z

src/v2/expressions.ts

@@ -0,0 +1,54 @@
+/*


Why is param expression specific to v2?

The code that currently allows users to define params in their functions code lives in the v2 namespace, so...I guess I just assumed?

I think recent discussion pointed to having params be top level module and not be v2 specific:

https://chat.google.com/room/AAAAMEMqN6Y/gowtrz2GD-Y

go/cf3-params-detailed is the doc that formally proposes ditching v2, but it seems easier for Victor to do that when he reaches a stopping point than having some PRs in v2 and some in root. Besides, technically that should be done in the launch.next branch, right? Technically we're changing an export path.

taeold · 2022-07-22T18:41:23Z

src/v2/expressions.ts

+ * The CLI does not currently have the capability to evaluate arbitrary CEL expressions.
+ * You almost certainly want to use FromParam() instead.
+ */
+export function FromCEL<T extends string | number | boolean>(


q: why is this fn necessary?

+1. Remove or @internal this too.

src/v2/options.ts

taeold · 2022-07-22T18:46:17Z

src/v2/options.ts

+ /** Region of the EventArc trigger. */
+ region?: Field<string>;
+
+ /** The service account that EventArc should use to invoke this function. Requires the P4SA to have ActAs permission on this service account. */
+ serviceAccount?: string | null;
+
+ /** The name of the channel where the function receives events. */
+ channel?: string;


I'm also not sure where these things are coming from and how it relates to the changes we are making. Especially region and serviceAccount - they are not the same thing as the ones we have defined in GlobalOptions and doesn't seem correct to me.

taeold · 2022-07-22T18:47:34Z

src/v2/providers/tasks.ts

+export { AuthData, Request };
+
+/** How congestion control should be applied to the function. */
+export interface ParameterizedRateLimits {


Why do we do this instead of wrapping RetryConfig/RateLimits in ../../common/providers/tasks instead?

Is params only supported in v2?

I'm in favor of letting v2 functions be configured with Params as well.

…over the wire

colerogers · 2022-08-03T14:38:08Z

spec/v2/providers/tasks.spec.ts

+ () => {}
+ );
+
+ expect(result.__trigger).to.deep.equal({


you can yank this, we don't use __trigger anymore

colerogers · 2022-08-03T14:40:59Z

src/v2/core.ts

- minInstances?: number;
- maxInstances?: number;
- availableMemoryMb?: number;
+ concurrency?: number | Expression<number>;


We don't use TriggerAnnotation anymore, so you can revert these changes

inlined

You're going to hate me for this but:

Please make an actual Expression class that has things like .val() and .toString() and .eq(test): BooleanExpression and a BooleanExpression class that implements then<T>(trueVal: T, falseVal: T): Expression<T>
In customer-facing SDKs I've been stricter about using typedefs for public types since they can worsen code complete. This is, for example, why I ripped out all typedefs for the handler function signatures in all of our handler code; it simply wasn't helping customers. I think Field<T> needs to be T | params.Expression<T> | ResetValue

inlined · 2022-08-10T23:08:10Z

spec/runtime/loader.spec.ts

 modulePath: './spec/fixtures/sources/commonjs-params',
- expected: { ...expected, params: [{ name: 'FOO', type: 'string' }] },
+ expected: { ...expected, params: [
+ { name: 'FOO', type: 'string' },


you, me, and @mbleigh really need to get on the same page for {type: 'select'} vs selectInput: { }.
We cannot release params until we are consistent here.

inlined · 2022-08-10T23:08:24Z

spec/v2/params.spec.ts


 describe('Param', () => {
 describe('#toSpec()', () => {
+ /*


remove the comments or remove the code.

you can also use it.skip if you intend to enable test at some time in the future.

inlined · 2022-08-10T23:09:48Z

src/v2/params/types.ts


 /** @hidden */
-type ParamValueType = 'string' | 'list' | 'boolean' | 'int' | 'float' | 'json';
+type ParamValueType = 'string' | 'list' | 'boolean' | 'int' | 'float' | 'json' | 'secret';


cut json for now

inlined · 2022-08-10T23:10:49Z

src/v2/params/types.ts

 const out: ParamSpec = {
 name: this.name,
 ...this.options,
 type: (this.constructor as typeof Param).type,


I personally wouldn't make SeecretParam inherit from Param because Param is an Expression and SecretParam is not.

inlined · 2022-08-10T23:14:51Z

src/v2/expressions.ts

+ * resolved to a value of the generic type parameter: i.e, you can pass
+ * an Expression<number> as the value of an option that normally accepts numbers.
+ */
+export type Expression<T extends string | number | boolean> = string; // eslint-disable-line


In the Functions codebase an Expression is not the same thing as T. Expression is a base class of Param but can (eventually) do a few extra things:

BooleanExpressions should subclass Expression and adds the ternary operator to create an Expression

All (non-boolean?) expressions should support an equality condition to make a BooleanExpression

inlined · 2022-08-10T23:18:23Z

src/v2/options.ts

+function isMemoryOption(
+ arg: MemoryOption | Expression<number>
+): arg is MemoryOption {
+ return arg in MemoryOptionToMB;


Isn't this going to fail for an Expression?

src/v2/options.ts

inlined · 2022-08-10T23:23:22Z

src/v2/providers/tasks.ts

+export { AuthData, Request };
+
+/** How congestion control should be applied to the function. */
+export interface ParameterizedRateLimits {


I'm in favor of letting v2 functions be configured with Params as well.

inlined · 2022-08-10T23:24:05Z

src/v2/providers/tasks.ts

 export interface TaskQueueOptions extends options.EventHandlerOptions {
 /** How a task should be retried in the event of a non-2xx return. */
- retryConfig?: RetryConfig;
+ retryConfig?: ParameterizedRetryConfig;


+1 to Daniel's comments. Just modify the normal RetryConfig and RateLimits to support Expressions.

TheIronDev · 2022-08-16T21:25:05Z

src/v2/expressions.ts

+export class IfElseExpression<
+ T extends string | number | boolean | string[]
+> extends Expression<T> {
+ test: Expression<boolean>;


Nit: this can be cleaned up a good bit with go/tsstyle#parameter-properties

other classes

src/v2/expressions.ts

TheIronDev · 2022-08-16T21:32:38Z

src/v2/expressions.ts

+export class CompareExpression<
+ T extends string | number | boolean | string[]
+> extends Expression<boolean> {
+ cmp: '==' | '>' | '>=' | '<' | '<=';


Maybe add a type to encapsulate this?

¯_(ツ)_/¯ I'm less concerned with that when the only consumer of this API is in the same file

yeah its definitely a very optional nit.

taeold

Just a few comments.

spec/fixtures/sources/commonjs-params/index.js

spec/runtime/loader.spec.ts

spec/fixtures/sources/commonjs-params/index.js

spec/runtime/manifest.spec.ts

taeold · 2022-08-16T21:48:18Z

spec/v2/params.spec.ts


 describe('Param', () => {
 describe('#toSpec()', () => {
+ /*


you can also use it.skip if you intend to enable test at some time in the future.

taeold · 2022-08-16T21:52:28Z

src/runtime/manifest.ts

 * An definition of a function as appears in the Manifest.
 */
-export interface ManifestEndpoint {
+export type ManifestEndpoint = {


ts question - why was interface => type necessary here?

Uh, it wasn't, I originally did something really dumb with another type that extended ManifestEndpoint (which does require the change), reverted it, and forgot to revert this.

src/runtime/manifest.ts

taeold · 2022-08-16T21:55:15Z

src/v2/expressions.ts

@@ -0,0 +1,54 @@
+/*


I think recent discussion pointed to having params be top level module and not be v2 specific:

https://chat.google.com/room/AAAAMEMqN6Y/gowtrz2GD-Y

src/v2/options.ts

src/v2/providers/database.ts

src/v2/expressions.ts

inlined · 2022-08-15T17:14:07Z

src/v2/expressions.ts

 /*
 * A CEL expression which can be evaulated during function deployment, and
 * resolved to a value of the generic type parameter: i.e, you can pass
 * an Expression<number> as the value of an option that normally accepts numbers.


expressions are only used in the context of params. They should be in the same file. You can have a separate file for expressions but v2/params should export everything you need to handle expressions.

inlined · 2022-08-17T01:24:03Z

src/v2/expressions.ts

 */
-export function ExprString<Expression>(arg: Expression): string {
- return (arg as unknown) as string;
+export class ParamExpression<


I originally tried this, but I found returning an Expression to be a dead end. For me, at least, I had to create new "Base" types for each expression so I could add the valid operators to them. So for example, a StringParam is a StringExpression, which implements eq in addition to being an Expression. The only time I gave up and returned Expression is the then API, but really I should go back and modify that to have N overrides so that each return type is a FooExpression and not an Expression.

src/v2/params/index.ts

src/v2/params/types.ts

inlined · 2022-08-17T01:36:22Z

src/v2/params/types.ts

 export type ParamOptions<T = unknown> = Omit<ParamSpec<T>, 'name' | 'type'>;

-export class Param<T = unknown> {
+export abstract class Param<T extends string | number | boolean | string[]> {


I want to play with abstract classes a bit to see if it changes my mind about the class hierarchy. I guess I don't mind having all expression types supporting the eq method even though I loose my sanity a smidge when I see "if (bool === true)". But Boolean expressions need to be their own type I think because only boolean expressions can implement then. So maybe.... create a boolean expression type that implements then and make sure eq returns it and that eq is on BooleanParam?

src/v2/params/types.ts

Berlioz · 2022-08-18T00:19:39Z

That latest commit made a fairly significant change to the underlying types of params/expressions to fit the more recent design doc. I'm going to resolve a ton of comments which basically reduce to "change the types/type signatures".

src/runtime/manifest.ts

TheIronDev · 2022-08-24T21:27:29Z

src/v2/expressions.ts

+ * resolved to a value of the generic type parameter: i.e, you can pass
+ * an Expression<number> as the value of an option that normally accepts numbers.
+ */
+export abstract class Expression<


In addition to this, I wonder if things would be cleaner if the two following existed

export type ExpressionOrType<T> = T | Expression<T>; export type NullableExpressionOrType<T> = ExpressionOrType<T> | null;

The reason I bring this up is because I saw an earlier instance of boolean | Expression<string> that may not have been intentional. This hardens the typing slightly.

WDYT?

src/v2/expressions.ts

src/v2/params/index.ts

…l oneof in protos

taeold

Minor nits on memory option, but I don't think that should be solved in the PR. this PR has stayed unmerge for too long!

taeold · 2022-08-25T21:43:23Z

src/runtime/manifest.ts

+ eventFilters: Record<string, string | Expression<string>>;
+ eventFilterPathPatterns?: Record<string, string | Expression<string>>;


I kinda don't get how Expression can be converted to Record<string, string>. Is this something that's handled by the CLI?

taeold · 2022-08-25T21:45:17Z

src/v2/options.ts

 * A value of null restores the defaults of 256MB.
 */
- memory?: MemoryOption | null;
+ memory?: MemoryOption | Expression<number> | null;


Aren't memoryOptions string?

firebase-functions/src/v2/options.ts

Line 56 in 27a4983

export type MemoryOption =

Ah I see. memory can either be a string or a number.

I think this is a bad footgun. For example, I would write something like this:

{ memory: "{{ isProd ? '1GB' : '256MB' }}" }

And this totally wouldn't work no? It would have to be instead:

{ memory: "{{ isProd ? 1024 : 256 }}" }

or something which feels surprising.

taeold · 2022-08-25T21:51:20Z

src/v2/params/index.ts

+ * @internal
+ */
+export function clearParams() {
+ declaredParams.splice(0, declaredParams.length);


Is this same as declaredParams = []?

It would be if declaredParams wasn't declared const.

hmm it's starting to sound like declaredParams isn't a constant variable afterall...

taeold · 2022-08-25T21:52:03Z

src/v2/params/index.ts

+ */
+export function defineSecretParam(
+ name: string
+ //options: ParamOptions<string> = {}


nit: Stray comment?

No longer touching v2/core.ts

wip: expr type

3ad62e2

Berlioz requested review from colerogers, inlined and taeold July 19, 2022 08:50

Berlioz mentioned this pull request Jul 19, 2022

WIP: present parameters in place of literal values for parameterized fields #1168

Closed

colerogers reviewed Jul 19, 2022

View reviewed changes

src/v2/expressions.ts Outdated Show resolved Hide resolved

src/v2/expressions.ts Outdated Show resolved Hide resolved

Berlioz added 3 commits July 19, 2022 12:04

npm run prettier

5f8cccb

trigger fields

b7f879b

more detailed expr type

48f2d9a

Berlioz changed the title ~~WIP: Insert Expression and Field types into GlobalOptions and inheriting types~~ Insert Expression and Field types into GlobalOptions and inheriting types Jul 21, 2022

Merge remote-tracking branch 'origin' into vf_expr_type

bf6c420

taeold requested changes Jul 22, 2022

View reviewed changes

Berlioz added 3 commits July 27, 2022 17:25

Merge remote-tracking branch 'origin' into vf_expr_type

b6d592e

wip: in-progress commit for providing params and param-valued fields …

e274fc7

…over the wire

wip: regexp validation

c00c2f7

colerogers previously requested changes Aug 3, 2022

View reviewed changes

Berlioz added 2 commits August 9, 2022 13:18

resourceInput selectors

66cffc9

remove extraneous changes to triggerannotation which is no longer in use

156a21d

inlined reviewed Aug 10, 2022

View reviewed changes

Berlioz added 3 commits August 11, 2022 16:04

wip class-ification of expression type

1089043

wip: split wire/manifest spec

b0a9063

WIP: classification of Expressions

8942e5c

TheIronDev reviewed Aug 16, 2022

View reviewed changes

src/v2/expressions.ts Outdated Show resolved Hide resolved

TheIronDev reviewed Aug 16, 2022

View reviewed changes

src/v2/expressions.ts Outdated Show resolved Hide resolved

TheIronDev reviewed Aug 16, 2022

View reviewed changes

src/v2/expressions.ts Outdated Show resolved Hide resolved

TheIronDev reviewed Aug 16, 2022

View reviewed changes

WIP: add value() semantics to params and expressions

3778deb

taeold reviewed Aug 16, 2022

View reviewed changes

inlined reviewed Aug 17, 2022

View reviewed changes

Berlioz added 2 commits August 17, 2022 15:53

type system changes to conform with go/cf3-params-detailed

6379cc3

style fixes per review

27937eb

removed shotgun testing change

df07167

TheIronDev reviewed Aug 24, 2022

View reviewed changes

src/runtime/manifest.ts Outdated Show resolved Hide resolved

TheIronDev reviewed Aug 24, 2022

View reviewed changes

src/v2/expressions.ts Outdated Show resolved Hide resolved

TheIronDev reviewed Aug 24, 2022

View reviewed changes

src/v2/params/index.ts Outdated Show resolved Hide resolved

Berlioz added 3 commits August 25, 2022 12:01

changes per review, most substantially making param inputs an optiona…

aaf3dda

…l oneof in protos

merge experssions.ts with params.ts

d4bf770

remove empty expressions.ts

a114386

taeold self-requested a review August 25, 2022 22:00

taeold approved these changes Aug 25, 2022

View reviewed changes

Berlioz added 5 commits August 29, 2022 14:34

Merge remote-tracking branch 'origin' into vf_expr_type

8c3447b

nits per pr

f63c895

typo

2c67693

format fix

251aa43

timezone -> timeZone, and fix SelectInput

47e2330

Berlioz merged commit d592847 into master Aug 30, 2022

		eventFilters: Record<string, string \| Expression<string>>;
		eventFilterPathPatterns?: Record<string, string \| Expression<string>>;

Insert Expression and Field types into GlobalOptions and inheriting types #1171

Insert Expression and Field types into GlobalOptions and inheriting types #1171

Uh oh!

Conversation

Berlioz commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

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

inlined left a 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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taeold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Berlioz commented Aug 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taeold left a 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

Choose a reason for hiding this comment

Berlioz commented Jul 19, 2022 •

edited

Loading