[expressions] changes fork to use namespacing

[ppisljar]

Summary

resolves #113379, #126472

Expressions.fork was updated to do namespacing internally. APIs stay the same. As part of the process getFunctions() had to be cleaned up and should now perform better.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[ppisljar]

@elasticmachine merge upstream

[ppisljar]

@elasticmachine merge upstream

[dokmic]

Using a decorator pattern here is a great idea. What do you think about breaking down the methods to make the class more straightforward?

I see the following points for this:

• If we make methods protected, we will be able to extend this class in the future if we have to diverge server or public implementation.
• Placing logic in separate methods should simplify readability and testability since we can test every method independently.
• In general, it's a good practice to make methods as straightforward as possible and have only one single responsibility. The setup and start are factory methods, so it's better to keep them only producing new instances.

export class ExpressionsServiceFork implements ExpressionServiceFork {
  constructor(private namespace: string, private expressions: ExpressionsService) {
    this.registerFunction = this.registerFunction.bind(this);
  }

  setup(): ExpressionsServiceSetup {
    return {
      ...this.expressionsService,
      registerFunction: this.registerFunction,
      // ...
    };
  }

  protected registerFunction(definition: /* ... */) {
    if (typeof definition === 'function') {
      definition = definition();
    }

    return this.expressionsService.registerFunction({
      ...definition,
      namespace: this.namespace,
    });
  }
}

Some notes regarding the example above:

• We should define protected methods as methods and not as properties so they will end up in the prototype. In this case, we will be able to use super in the extended class.
• We should bind methods in the constructor to prevent producing new boundaries on every start/setup call.

[ppisljar]

• I don't think this will be extended internally and it shouldn't be extended externally.
• we don't want to make the methods visible on the ExpressionServiceFork class, if we make them protected how can we test this from the outside then ?
• for me personally in this specific case readability is better as it is now, makes it obvious that the fork is just a wrapper around ExpressionsService and we don't need to do the binding

[dokmic]

I don't think this will be extended internally and it shouldn't be extended externally.

Well, we cannot predict that. That's not about the extension necessity but more about abstraction. Those methods seem to be like internal API and not an implementation detail. Even though we export only start and setup, they define the forked service.

we don't want to make the methods visible on the ExpressionServiceFork class, if we make them protected how can we test this from the outside then ?

That's possible to call them through an extended anonymous class. But in cases like that, we can just put related tests under the describe('method') block since it is not a private method.

for me personally in this specific case readability is better as it is now, makes it obvious that the fork is just a wrapper around ExpressionsService and we don't need to do the binding

I think it's better if we can extract those into methods. It should not make it less straightforward, but it will be more consistent with the ExpressionsService. The latter makes sense because we are actually forking that service.
If we do that, it will make the decorator pattern more visible so that it still should be clear that the class is a wrapper.

[ppisljar]

@elasticmachine merge upstream

[ppisljar]

@elasticmachine merge upstream

[dokmic]

Great job! LGTM 👍

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 77ae63b
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
expressions	171	172	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
expressions	1647	1708	+61

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
canvas	1.0MB	1.0MB	+24.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
expressions	4	5	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
expressions	87.5KB	88.9KB	+1.5KB

Unknown metric groups

API count

id	before	after	diff
expressions	2094	2153	+59

History

• 💔 Build #30317 failed 0fc33f1
• 💔 Build #30313 failed ce05394
• 💚 Build #29886 succeeded bb0ca6b
• 💚 Build #29716 succeeded 10c558a
• 💔 Build #29438 failed 0041e3b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[crob611]

Presentation changes LGTM 👍

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125957 or prevent reminders by adding the backport:skip label.

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125957 or prevent reminders by adding the backport:skip label.