Skip to content

Allow users to present String and Integer params in a ManifestSpec #1166

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

Merged
merged 6 commits into from
Jul 19, 2022
Merged

Allow users to present String and Integer params in a ManifestSpec #1166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
54e0e9c
allow users to present String and Integer params in a ManifestSpec
Berlioz Jul 7, 2022
a38bad7
format:fix
Berlioz Jul 7, 2022
6887a72
fix bad copy-paste in fixture
Berlioz Jul 7, 2022
6ab879c
fixes for code review, especially reverting changes to visibility in …
Berlioz Jul 14, 2022
8eb1b78
more visibility fixes
Berlioz Jul 14, 2022
626edc7
Merge branch 'master' into vf_present_params
Berlioz Jul 19, 2022
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
21 changes: 21 additions & 0 deletions spec/fixtures/sources/commonjs-params/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const functions = require("../../../../src/index");
const functionsv2 = require("../../../../src/v2/index");
const { defineString } = require("../../../../src/v2/params");

defineString("FOO");

exports.v1http = functions.https.onRequest((req, resp) => {
resp.status(200).send("PASS");
Copy link
Member

@inlined inlined Jul 8, 2022

Choose a reason for hiding this comment

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

Nit: you don't need to send a 200 status; it's inferred if you just say send

});

exports.v1callable = functions.https.onCall(() => {
return "PASS";
});

exports.v2http = functionsv2.https.onRequest((req, resp) => {
resp.status(200).send("PASS");
});

exports.v2callable = functionsv2.https.onCall(() => {
return "PASS";
});
3 changes: 3 additions & 0 deletions spec/fixtures/sources/commonjs-params/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "commonjs-params"
}
5 changes: 5 additions & 0 deletions spec/runtime/loader.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,11 @@ describe('loadStack', () => {
},
},
},
{
name: 'has params',
modulePath: './spec/fixtures/sources/commonjs-params',
expected: { ...expected, params: [{ name: 'FOO', type: 'string' }] },
Copy link
Member

@inlined inlined Aug 5, 2022

Choose a reason for hiding this comment

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

@mbleigh It still feels wrong to me that a param has a discriminating union field and input doesn't.

Why is it

{
  name: "FOO",
  type: "string",
  input: {
    textInput: {}
  }
}

and not

{
  name: "FOO",
  type: "string",
  input: {
    type: "text",
  }
}

or even

{
  stringParam: {
    name: "FOO",
    type: "string",
    input: {
      textInput: {}
    }
  }
}

It seems like those would be more ideologically consistent.

Copy link
Contributor

@mbleigh mbleigh Aug 6, 2022

Choose a reason for hiding this comment

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

Because one is defining a small enumerated set of types to which a string value can be cast, and the other is defining an unbounded set of configuration and validation hints for client UIs.

The main reason I care is because I don't want us to end up with a giant grab-bag of fields with insanely overcomplicated "if type is X then fields A, B, D, and F are applicable. if type is Y then fields A, D, F, G, and K are applicable".

The cast type has no additional configuration, so it works fine as a simple enum. Likewise if the input type had no additional configuration, it would be fine as a simple enum. But we aim to have many different types of inputs (checkbox, radio group, dropdown, input, text field, resource picker, secret picker, more as we need) and each of those will have some amount of configuration.

I look at a oneof as basically being an "enum with configuration", which is why I don't find this API inconsistent.

Copy link
Member

@inlined inlined Aug 10, 2022

Choose a reason for hiding this comment

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

I like Scala's term for "enum with configuration"--case class.

I see some of your argument, though the last example (where each is a oneof message) follows the argument even further. In the proposal, all input types can be used with all parameter types, but this isn't true. As I understand the current proposal, multiselect input can only be used with a list param (which can only use a multiselect input I think). And a text input can only use certain validators depending on their param type. While the last format makes my stomach churn instinctively, it has the benefit that the type system only allows the right input to be picked with the right param type.

},
];

for (const tc of testcases) {
Expand Down
8 changes: 7 additions & 1 deletion src/runtime/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import {
ManifestStack,
} from './manifest';

import * as params from '../v2/params';

/**
* Dynamically load import function to prevent TypeScript from
* transpiling into a require.
Expand Down Expand Up @@ -112,9 +114,13 @@ export async function loadStack(functionsDir: string): Promise<ManifestStack> {

extractStack(mod, endpoints, requiredAPIs);

return {
const stack: ManifestStack = {
endpoints,
specVersion: 'v1alpha1',
requiredAPIs: mergeRequiredAPIs(requiredAPIs),
};
if (params.declaredParams.length > 0) {
stack.params = params.declaredParams.map((p) => p.toSpec());
}
return stack;
}
3 changes: 3 additions & 0 deletions src/runtime/manifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
// SOFTWARE.

import { ParamSpec } from '../v2/params/types';

/**
* An definition of a function as appears in the Manifest.
*/
Expand Down Expand Up @@ -87,6 +89,7 @@ export interface ManifestRequiredAPI {
*/
export interface ManifestStack {
specVersion: 'v1alpha1';
params?: ParamSpec[];
requiredAPIs: ManifestRequiredAPI[];
endpoints: Record<string, ManifestEndpoint>;
}
1 change: 1 addition & 0 deletions src/v2/params/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* @hidden
* @alpha
*/

import {
BooleanParam,
FloatParam,
Expand Down
23 changes: 10 additions & 13 deletions src/v2/params/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,13 @@ export interface ParamSpec<T = unknown> {
default?: T;
label?: string;
description?: string;
valueType?: ParamValueType;
type: ParamValueType;
Copy link
Contributor

@taeold taeold Jul 8, 2022

Choose a reason for hiding this comment

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

nit* If we are going to change the name, should we also change the name of type ParamValueType to type ParamType?

}

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

export class Param<T = unknown> {
static valueType: ParamValueType = 'string';
static type: ParamValueType = 'string';

constructor(readonly name: string, readonly options: ParamOptions<T> = {}) {}

Expand All @@ -61,7 +58,7 @@ export class Param<T = unknown> {
const out: ParamSpec = {
name: this.name,
...this.options,
valueType: (this.constructor as typeof Param).valueType,
type: (this.constructor as typeof Param).type,
};
if (this.options.default && typeof this.options.default !== 'string') {
out.default = (this.options.default as
Expand All @@ -78,7 +75,7 @@ export class StringParam extends Param<string> {
}

export class IntParam extends Param<number> {
static valueType: ParamValueType = 'int';
static type: ParamValueType = 'int';

get value(): number {
const intVal = parseInt(
Expand All @@ -97,7 +94,7 @@ export class IntParam extends Param<number> {
}

export class FloatParam extends Param<number> {
static valueType: ParamValueType = 'float';
static type: ParamValueType = 'float';

get value(): number {
const floatVal = parseFloat(
Expand All @@ -115,7 +112,7 @@ export class FloatParam extends Param<number> {
}

export class BooleanParam extends Param {
static valueType: ParamValueType = 'boolean';
static type: ParamValueType = 'boolean';

get value(): boolean {
const lowerVal = (
Expand All @@ -137,7 +134,7 @@ export class BooleanParam extends Param {
}

export class ListParam extends Param<string[]> {
static valueType: ParamValueType = 'list';
static type: ParamValueType = 'list';

get value(): string[] {
return typeof this.rawValue === 'string'
Expand All @@ -148,7 +145,7 @@ export class ListParam extends Param<string[]> {
toSpec(): ParamSpec<string> {
const out: ParamSpec = {
name: this.name,
valueType: 'list',
type: 'list',
...this.options,
};
if (this.options.default && this.options.default.length > 0) {
Expand All @@ -160,7 +157,7 @@ export class ListParam extends Param<string[]> {
}

export class JSONParam<T = any> extends Param<T> {
static valueType: ParamValueType = 'json';
static type: ParamValueType = 'json';

get value(): T {
if (this.rawValue) {
Expand Down