Skip to content
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.

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

remove expressions fork in favor of namespacing #113379

Closed
Tracked by #46909
ppisljar opened this issue Sep 29, 2021 · 0 comments · Fixed by #125957
Closed
Tracked by #46909

remove expressions fork in favor of namespacing #113379

ppisljar opened this issue Sep 29, 2021 · 0 comments · Fixed by #125957
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort

Comments

@ppisljar
Copy link
Member

ppisljar commented Sep 29, 2021

Expressions service exposes a fork function, which creates a copy of current expression service and allows consumer to keep their isolated version. The main intention behind that was to allow apps to have isolated functions which don't pollute the global functions registry.

There are several drawbacks to the current approach:

  • memory overhead, due to keeping some functions in multiple registries
  • cpu overhead: getFunctions() call is expensive as its copying all the functions to a new object
  • maintenance overhead: if you are using fork you will need to add your own alert types, report types etc to get functionality which you would otherwise get out of the box.

In order to fix all drawbacks we will change internal implementations from forks to namespacing.

   const expressionFork = expressions.fork('fork-name'); // forks will need to provide a name
   const forkSetup = expressionFork.setup();
   forkSetup.registerFunction(myFn);
   const forkStart = expressionFork.start();
   // from now on i can no longer call any method on forkSetup
   const result = await forkStart.run('my expression');

For the consumers there will be a few changes to the API:

  • fork function will need to be called with a name: string parameter
  • fork will no longer return an object with all the functions, but an object with setup and start which will return appropriate contracts
  • registering functions after start will no longer be possible

ExpressionFunctionDefinition will be extended with: namespace?: string.

ExpressionExecutionParams will also be extended with namespace?: string

Logic of expression executor will change so that:

  • running expressions without namespace provided will only use functions with no namespace
  • running expressions with namespace provided will use functions with no namespace as well as functions with provided namespace. in case of collision function with provided namespace name gets the preference.

Logic inside fork will also change so that:

  • fork will no longer keep a registry, but just its name.
  • registerFunction will internally call expressions.registerFunction({ ...fnDef, namespace })
  • execute/run will internally call expressions.execute(expression, input, { ...params, namespace })
@ppisljar ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppServicesSv labels Sep 29, 2021
@ppisljar ppisljar mentioned this issue Sep 29, 2021
20 tasks
@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort loe:large Large Level of Effort and removed loe:small Small Level of Effort labels Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:large Large Level of Effort
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant