feat: enable plugin mechanism for async func #70

YADROOKIE · 2022-07-20T14:28:52Z

complete async function plugin
Signed-off-by: yad 459647480@qq.com

Signed-off-by: yad <459647480@qq.com>

mergeable · 2022-07-20T14:29:43Z

Thanks for creating a pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

webup

Seems there would be some structural level code refactoring, so let me put all my comments at below trying to give you a whole picture.

Below items are listed in priority, from the most to least important.

Where comes the PLUGIN_CONTEXT?
- Taking context_test.go for reference, all of the context data are stored in FUNC_CONTEXT.
- Accordingly, plugin_cotext.ts is NO need to exist, changes in options.ts is redundant, simply adding some plugin related fields in OpenFunctionCotext is good enough. (Check Context structure in plugin mechanism for reference as well)
- Nevertheless, we need some Plugin class to deal with plugin instance so as to hold data and provide instance methods like get shown in this demo.
How to store plugins?
- A map structure is already an good structure to hold and access both plugin name as key and function instance as value, so why we have to need prePlugins / prePluginFuncs and the like to hold keys and values separately?
- BTW, in ts, Record is a more standard type to hold objects, rather than Map.
Try to take more advantage of well-known Node.js libraries
- console.error should be replaced by debug (could search usage in files)
- for (let) loop should be replaced by Lodash collection utilities which will help ensure the safe execution and returns
Finally, pay a little attention to import sequence, usually we group imports from the same level together, and common sequence is the farther the earlier. For example, node built-in libs -> 3rd-party libs -> files in grandparent folder -> files in parent folder -> side-by-side files, etc.

YADROOKIE · 2022-07-21T10:45:42Z

I got it , prePlugins & postPlugin is in order to ensure execution sequence

webup · 2022-07-21T13:35:55Z

I got it , prePlugins & postPlugin is in order to ensure execution sequence

So you might use a collection of plugin objects for "pre & post" separately to keep the sequence with plugin data encapsulated into a class object.

Signed-off-by: yad <459647480@qq.com>

…unctions-framework-nodejs into feature/plugin-async

Signed-off-by: yad <459647480@qq.com>

YADROOKIE · 2022-07-22T09:09:29Z

Im refactor code, but lodash collection utilities is aysnc ，so I still use for loop to ensure plugin hook sync ;

webup · 2022-07-22T10:46:47Z

Im refactor code, but lodash collection utilities is aysnc ，so I still use for loop to ensure plugin hook sync ;

Oh, you are right, may try await array.reduce to keep the sequence, for example.

Signed-off-by: yad <459647480@qq.com>

YADROOKIE · 2022-07-25T03:09:47Z

improve the code & add unit test

webup

@YADROOKIE I'm thinking why shouldn't we fallback all methods for a user plugin function, so that no need to do lots of fields & methods checking?

It looks better to attach default get / execXXX methods to plugin function prototype instead rudely telling user you must have blablabla otherwise your function wouldn't work. What do you think? :)

webup · 2022-07-25T07:57:50Z

src/loader.ts

@@ -92,6 +95,7 @@ export async function getUserFunction(
 } | null> {
  try {
    const functionModulePath = getFunctionModulePath(codeLocation);
+    console.log(functionModulePath);


Don't forget to remove console.log, or use debug instead.

webup · 2022-07-25T08:02:19Z

src/options.ts

@@ -134,7 +134,6 @@ const FunctionContextOption = new ConfigurableOption(
    }
  }
 );
-


Why remove this line?

webup · 2022-07-25T08:09:52Z

src/loader.ts

+  if (
+    options.context &&
+    options.context.prePlugins &&
+    options.context.postPlugins


Take better advantage of optional chaining, i.e. options.context?.postPlugins

Why we should ensure both pre and post plugins existed? Seems it's better to make another helper function to encapsulate common logics and handle pre and post plugins separately.

webup · 2022-07-25T08:11:28Z

src/loader.ts

+    options.context.prePlugins &&
+    options.context.postPlugins
+  ) {
+    options.context.prePlugins.forEach(item => {


As mentioned in our last call, use lodash utilities instead to ensure safe invocation and returns.

Use plugin instead of item, that's more meaningful.

webup · 2022-07-25T08:14:40Z

src/loader.ts

+
+    try {
+      // load plugin js files
+      const instances: Map<string, Plugin> = new Map();


Why not simply using {} instead of Map?

It can clear type

It can clear type

So why not using Record?

webup · 2022-07-25T09:37:14Z

test/data/plugins/errorMissGet.js

+    static Name = "error-miss-get-plugin"
+
+    execPreHook(ctx){
+        console.log(`-----------error plugin pre hook-----------`)


Please make sure or indent contains only 2 spaces.

webup · 2022-07-25T09:44:33Z

test/data/plugins/plugindemo.js

@@ -0,0 +1,33 @@
+const { resolve } = require("path")


Use import instead of require, let's keep the same coding style.

webup · 2022-07-25T09:45:25Z

test/data/plugins/plugindemo.js

+    constructor(){
+        console.log(`init demo plugins`)
+    }
+    sleep(){


Better to make it as a helper function out of the class.

webup · 2022-07-25T09:46:21Z

test/data/plugins/plugindemo.js

+    async execPreHook(ctx){
+        console.log(`-----------demo plugin pre hook-----------`)
+        ctx['pre'] = 'pre-exec';
+        await this.sleep()


Please ensure all sentences in the project ends with a semicolon.

webup · 2022-07-25T09:57:29Z

test/integration/async_server_plugin.ts

+import {FrameworkOptions} from '../../src/options';
+import assert = require('assert');
+
+const TEST_CONTEXT: OpenFunctionContext = {


Too much alike with async_server, it's better to combine the test cases to make better use of the common data and logics.

At first I was combine the test cases with async_server ,but it will conflict make it single file finally

In this case, it's better to extract common test data into a separate file for reuse.

it seem still have conflict in when data in a common file

it seem still have conflict in when data in a common file

Please describe the problem or details, it's hard to provide help based on "it seems".

I was in the wrong position before ， it is ok now

Signed-off-by: yad <459647480@qq.com>

…d replace Map Signed-off-by: yad <459647480@qq.com>

Signed-off-by: yad <459647480@qq.com>

webup

Generally speaking, problems addressed in the latest review have been resolved, so it's okay to make a merge.

Signed-off-by: yad <459647480@qq.com>

webup · 2022-08-04T07:56:38Z

@all-contributors please add @YADROOKIE for code contribution

allcontributors · 2022-08-04T07:56:47Z

@webup

I've put up a pull request to add @YADROOKIE! 🎉

✨ feat: enable asycn function plugin

56ff7ad

Signed-off-by: yad <459647480@qq.com>

auto-assign bot requested a review from webup July 20, 2022 14:28

auto-assign bot assigned YADROOKIE Jul 20, 2022

webup changed the title ~~✨ feat: enable asycn function plugin~~ feat: enable asycn function plugin Jul 20, 2022

webup linked an issue Jul 20, 2022 that may be closed by this pull request

Enable function plugin mechanism #46

Closed

webup added the type: feature New feature or request label Jul 20, 2022

YADROOKIE marked this pull request as draft July 20, 2022 14:50

webup requested changes Jul 21, 2022

View reviewed changes

YADROOKIE and others added 4 commits July 22, 2022 16:36

♻️ refactor enable aysnc function plugin

10c240f

Signed-off-by: yad <459647480@qq.com>

Merge branch 'OpenFunction:master' into feature/plugin-async

7034730

Merge branch 'feature/plugin-async' of https://github.com/YADROOKIE/f…

be20c3f

…unctions-framework-nodejs into feature/plugin-async

🎨 formate code

c2146ee

Signed-off-by: yad <459647480@qq.com>

YADROOKIE requested a review from webup July 22, 2022 08:47

YADROOKIE marked this pull request as ready for review July 22, 2022 09:05

YADROOKIE marked this pull request as draft July 22, 2022 09:05

YADROOKIE added 2 commits July 23, 2022 16:48

🎨 improve aysnc function plugin code

7c1d9e8

Signed-off-by: yad <459647480@qq.com>

🎨 rename test file gi

c844496

Signed-off-by: yad <459647480@qq.com>

YADROOKIE marked this pull request as ready for review July 23, 2022 08:59

auto-assign bot requested a review from benjaminhuo July 23, 2022 08:59

YADROOKIE marked this pull request as draft July 23, 2022 08:59

webup requested changes Jul 25, 2022

View reviewed changes

🎨 improve & formate code

af1ac61

Signed-off-by: yad <459647480@qq.com>

YADROOKIE requested a review from webup July 25, 2022 11:44

🎨 format code

4bb5ad6

Signed-off-by: yad <459647480@qq.com>

YADROOKIE added 2 commits July 25, 2022 20:50

🎨 extract common test data into a separate file for reuse & use Recor…

0d5ceb3

…d replace Map Signed-off-by: yad <459647480@qq.com>

🎨 delete test data

184c2a9

Signed-off-by: yad <459647480@qq.com>

YADROOKIE marked this pull request as ready for review July 26, 2022 02:32

YADROOKIE marked this pull request as draft July 26, 2022 02:32

YADROOKIE and others added 10 commits July 27, 2022 19:33

🎨 test data in common file

eae45d8

Signed-off-by: yad <459647480@qq.com>

🎨 remove annotation

a95dad2

Signed-off-by: yad <459647480@qq.com>

Merge branch 'OpenFunction:master' into feature/plugin-async

b6d1c50

Merge branch 'OpenFunction:master' into feature/plugin-async

1c9b4a8

Merge branch 'OpenFunction:master' into feature/plugin-async

48946da

🎨 improve the codegit

a2f2db7

Signed-off-by: yad <459647480@qq.com>

🎨 imporove plugin loader

fa7c1f5

Signed-off-by: yad <459647480@qq.com>

process conflict

37a91af

Signed-off-by: yad <459647480@qq.com>

Merge branch 'OpenFunction:master' into feature/plugin-async

c1074ad

resolve conflict

75d2422

Signed-off-by: yad <459647480@qq.com>

YADROOKIE marked this pull request as ready for review August 3, 2022 11:08

YADROOKIE marked this pull request as draft August 3, 2022 11:08

YADROOKIE marked this pull request as ready for review August 3, 2022 23:05

webup approved these changes Aug 4, 2022

View reviewed changes

add api docs

8403803

Signed-off-by: yad <459647480@qq.com>

webup changed the title ~~feat: enable asycn function plugin~~ feat: enable async function plugin Aug 4, 2022

webup changed the title ~~feat: enable async function plugin~~ ✨ feat: enable plugin mechanism for async func Aug 4, 2022

webup merged commit 5d91417 into OpenFunction:master Aug 4, 2022

allcontributors bot mentioned this pull request Aug 4, 2022

docs: add YADROOKIE as a contributor for code #76

Merged

webup changed the title ~~✨ feat: enable plugin mechanism for async func~~ feat: enable plugin mechanism for async func Aug 4, 2022

@@ @@ -134,7 +134,6 @@ const FunctionContextOption = new ConfigurableOption( @@
                   }
                 }
               );

feat: enable plugin mechanism for async func #70

feat: enable plugin mechanism for async func #70

Uh oh!

Conversation

YADROOKIE commented Jul 20, 2022

Uh oh!

mergeable bot commented Jul 20, 2022

Uh oh!

webup left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YADROOKIE commented Jul 21, 2022

Uh oh!

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

Uh oh!

Uh oh!

YADROOKIE commented Jul 22, 2022

Uh oh!

webup commented Jul 22, 2022

Uh oh!

YADROOKIE commented Jul 25, 2022

Uh oh!

webup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webup left a comment

Choose a reason for hiding this comment

Uh oh!

webup commented Aug 4, 2022

Uh oh!

allcontributors bot commented Aug 4, 2022

Uh oh!

Uh oh!

webup left a comment •

edited

Loading

webup commented Jul 21, 2022 •

edited

Loading

webup Jul 25, 2022 •

edited

Loading