fix #4: Removes 'function' prefix from CLI args #16

grant · 2019-04-11T18:18:36Z

Fixes #4.

Removes redundant function from CLI/env vars for target and signature-type
Updates docs
Moves string literals into constants to reduce renaming multiple pieces of code and clearly define the library interface in 1 place.

swalkowski · 2019-04-15T06:21:53Z

src/index.ts

 enum NodeEnv {
  PRODUCTION = 'production',
 }

 const argv = minimist(process.argv, {
-  string: ['port', 'function-target', 'function-signature-type'],
+  string: [ENV_VARIABLE.PORT, FLAG.TARGET, FLAG.SIGNATURE_TYPE],


Should be FLAG.PORT.

swalkowski · 2019-04-15T06:22:29Z

src/index.ts

 });

 const CODE_LOCATION = process.cwd();
-const PORT = argv['port'] || process.env.PORT || '8080';
+const PORT =
+  argv[ENV_VARIABLE.PORT] || process.env[ENV_VARIABLE.PORT] || '8080';


Should be argv[FLAG.PORT].

swalkowski · 2019-04-15T06:24:55Z

src/index.ts

-  argv['function-signature-type'] ||
-  process.env.FUNCTION_SIGNATURE_TYPE ||
-  'http';
+  argv[FLAG.SIGNATURE_TYPE] || process.env.FUNCTION_SIGNATURE_TYPE || 'http';


Should be process.env[ENV_VARIABLE.SIGNATURE_TYPE], for consistency.

swalkowski · 2019-04-15T06:28:44Z

src/index.ts

+
+// Supported environment variables
+const ENV_VARIABLE = {
+  PORT: 'FUNCTION_PORT',


This should be kept as PORT. It is not a functions-specific environment variable, and it comes from Knative runtime contract.

swalkowski · 2019-04-15T07:09:51Z

src/index.ts

+const ENV_VARIABLE = {
+  PORT: 'FUNCTION_PORT',
+  TARGET: 'FUNCTION_TARGET',
+  SIGNATURE_TYPE: 'FUNCTION_SIGNATURE_TYPE', // underscore


While we are refactoring this, please replace FUNCTION_SIGNATURE_TYPE in FUNCTION_SIGNATURE_TYPE must be one of 'http' or 'event'. message below with Function signature type, as it can refer to either flag or env var.

swalkowski · 2019-04-15T07:17:59Z

src/index.ts

 });

 const CODE_LOCATION = process.cwd();
-const PORT = argv['port'] || process.env.PORT || '8080';
+const PORT =
+  argv[ENV_VARIABLE.PORT] || process.env[ENV_VARIABLE.PORT] || '8080';
 const FUNCTION_TARGET =


While I can see how the refactoring you are proposing makes sense, it also makes the code a bit less readable, in my opinion, because of the extra mappings. For example, we are mapping FUNCTION_TARGET string to ENV_VARIABLE.TARGET and then resolving it to FUNCTION_TARGET const.

If the names without 'function' prefix are to be the canonical ones, then let's drop FUNCTION_ prefix from FUNCTION_TARGET, FUNCTION_SIGNATURE_TYPE_STRING, FUNCTION_SIGNATURE_TYPE consts.

Dropped the FUNCTION_ prefix.

swalkowski

I like this change now, please address the remaining comments. Thanks!

swalkowski · 2019-04-16T06:24:21Z

src/index.ts

-const FUNCTION_TARGET =
-  argv['function-target'] || process.env.FUNCTION_TARGET || 'function';
+const PORT = argv[FLAG.PORT] || process.env[ENV.PORT] || '8080';
+const TARGET = argv[FLAG.TARGET] || process.env[ENV.TARGET] || 'function';

 const FUNCTION_SIGNATURE_TYPE_STRING =


We could drop FUNCTION_ prefix in this const as well, for consistency and brevity.

This request is getting out of scope, but I've changed it as I agree.

swalkowski · 2019-04-16T06:24:27Z

src/index.ts

-  argv['function-signature-type'] ||
-  process.env.FUNCTION_SIGNATURE_TYPE ||
-  'http';
+  argv[FLAG.SIGNATURE_TYPE] || process.env[ENV.SIGNATURE_TYPE] || 'http';
 const FUNCTION_SIGNATURE_TYPE =


We could drop FUNCTION_ prefix in this const as well, for consistency and brevity.

swalkowski · 2019-04-16T06:27:10Z

src/index.ts

+// Supported environment variables
+const ENV = {
+  PORT: 'PORT',
+  TARGET: 'TARGET',


Perhaps I wasn't clear in my previous comments: we should keep the environment variable names unchanged, just like in the table in README. Please keep PORT, FUNCTION_TARGET, FUNCTION_SIGNATURE_TYPE.

Short names are fine for flag names and const names in the code.

I mean:

const ENV = { PORT: 'PORT', TARGET: 'FUNCTION_TARGET', SIGNATURE_TYPE: 'FUNCTION_SIGNATURE_TYPE', // underscore };

Yes. Agreed.
Not sure why I changed this. Reverted the FUNCTION_ prefix for these env vars.

grant

Addressed comments. PTAL.

grant · 2019-04-16T17:09:12Z

src/invoker.ts

 import * as http from 'http';
+import * as onFinished from 'on-finished';


IDE auto-sorted.

grant · 2019-04-16T17:09:29Z

test/invoker.ts

 import * as invoker from '../src/invoker';
+import * as supertest from 'supertest';


IDE auto-sorted.

swalkowski

Thank you! LGTM.

fix #4: Removes 'function' prefix from CLI arg and env vars

e0b3bc0

grant requested a review from swalkowski April 11, 2019 18:18

googlebot added the cla: yes label Apr 11, 2019

fix: Re-add 'FUNCTION_' to env var prefix

ef41c44

swalkowski reviewed Apr 15, 2019

View reviewed changes

grant added 3 commits April 15, 2019 11:23

fix: Fix flag/env setup

12e978c

fix: docs comment

8c25e1b

Merge

49e0dd3

swalkowski reviewed Apr 16, 2019

View reviewed changes

fix: Update FUNCTION_ prefix for env var but not consts

e4230b7

grant commented Apr 16, 2019

View reviewed changes

swalkowski approved these changes Apr 17, 2019

View reviewed changes

swalkowski changed the title ~~fix #4: Removes 'function' prefix from CLI arg and env vars~~ fix #4: Removes 'function' prefix from CLI args Apr 17, 2019

swalkowski merged commit 76f134b into master Apr 17, 2019

grant deleted the grant_remove_function_prefix branch April 17, 2019 15:24

grant restored the grant_remove_function_prefix branch May 31, 2019 17:21

grant deleted the grant_remove_function_prefix branch May 6, 2020 22:04

G0dh mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities G0dh/functions-framework-nodejs#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #4: Removes 'function' prefix from CLI args #16

fix #4: Removes 'function' prefix from CLI args #16

grant commented Apr 11, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski Apr 15, 2019

grant Apr 15, 2019

swalkowski left a comment

swalkowski Apr 16, 2019

grant Apr 16, 2019

swalkowski Apr 16, 2019

grant Apr 16, 2019

swalkowski Apr 16, 2019

swalkowski Apr 16, 2019

grant Apr 16, 2019

grant left a comment

grant Apr 16, 2019

grant Apr 16, 2019

swalkowski left a comment

		import * as http from 'http';
		import * as onFinished from 'on-finished';

		import * as invoker from '../src/invoker';
		import * as supertest from 'supertest';

fix #4: Removes 'function' prefix from CLI args #16

fix #4: Removes 'function' prefix from CLI args #16

Conversation

grant commented Apr 11, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swalkowski 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

grant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swalkowski left a comment

Choose a reason for hiding this comment