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

Async registries #65993

Closed
Dosant opened this issue May 11, 2020 · 4 comments
Closed

Async registries #65993

Dosant opened this issue May 11, 2020 · 4 comments
Labels
discuss impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Meta technical debt Improvement of the software architecture and operational architecture

Comments

@Dosant
Copy link
Contributor

Dosant commented May 11, 2020

We want to introduce a pattern for various app-services registries that should encourage lazy loading:

We need:

  • A common interface and guidelines to follow when creating our registries
  • It should encourage lazy loading of registry items. We need to reduce our bundle sizes
  • We should have a way to preload multiple registry items on demand. This is needed to support sync API on a registry
  • sync API support is needed for the server side to be used in migrate/extract/inject functions. A registry should integrate with PerstistableState concepts
  • [maybe] We need to investigate a way to load registry items in a single request. We should decide if it is worth it at all.
  • [maybe] Figure out a way how to DYNAMICALLY share code between client and server. (need for persistable state migrations)
Old context

Following-up on: #64179 (comment)

We need to work on code-splitting app-arch code,

Important question since we own multiple registries, but usually don't own actual registered items:
Should we explore into direction of async registries which would enforce code-splitting practices? or should we instead pragmatically fix existing issues inside registered items code?

So 2 options:

  1. Explore possibility of async registries, try to enforce lazy-loading on registry items?
    --or--
  2. Fix issues ad-hoc in specific registry item implementation

Exploring async registries

To explore this direction a bit I tried to make uiActions registry async:
I went with something like this:

public readonly registerAction = <A extends ActionDefinition>(definition: A): Action<ActionContext<A>> {
 // ...
}

we could have:

public readonly registerAction = <A extends ActionDefinition>(actionId: string, definition: () => Promise<A>): (() => Promise<Action<ActionContext<A>>>) {
 // ...
}

and when registering an action:

uiActions.registerAction('ACTION_HELLO_WORLD', async () =>
      (await import('./hello_world_action_lazy')).createHelloWorldAction(async () => ({
        openModal: (await core.getStartServices())[0].overlays.openModal,
      }))
    );

#65991

The idea here that item is registered, but initialised only when it is accessed for the first time

questions / concerns :

  1. Does it makes sense at all to approach it on registry level? Anyway only thing we could do is to make sure our apis are async, but it is up to developer who uses registry to use import()?

  2. Could this LAZY_LOAD_EVERYTHING approach cause more problems then improvements? Not clear how for example: to bundle all canvas specific expression function in one bundle.

  3. Just having and id available during registration could be not enough. we'd also likely need displayName, for example, to be able to display registry items in a list without loading all the data

  4. If we decide to go with something like this, should we have BaseRegistry interface which every our registry would implement. (Or base class). it could take care of caching / preloading and give common structure for all our registries?

Fixing issues ad-hoc:

Example prs to give an idea how it will look like:

  1. Lazy loading Vega related deps: Reduce visTypeVega bundle size #64749
  2. Lazy loading table-vis: https://github.com/elastic/kibana/pull/64778/files

Maybe such pragmatic approach is the way to go?
We just will address left problematic places and make sure our docs / example are following best practices?
With planned work to notify about bundle size issue introduced in new prs following this best practices will become easier: #62263

@kibanamachine kibanamachine added this to To triage in kibana-app-arch May 11, 2020
@Dosant Dosant added the Meta label May 11, 2020
@Dosant Dosant changed the title [discuss] Async registries? [discuss] Code splitting AppArch registries? May 11, 2020
@lukeelmers
Copy link
Member

Moving toward async registries would also give us longer term design flexibility, and could help solve some other problems unrelated to lazy loading (like making it easier to use core.getStartServices, for example, or synchronizing client-side registry items with the server).

We have talked before about some sort of common interface or registry base class, and have debated what that should look like in practice and exactly how prescriptive it should be.

It is starting to feel like we should revisit this topic more seriously, especially in light of related discussions like @stacey-gammon’s thread on an enhancements pattern. Both of these could be solved with a common registry implementation.

Most important here IMHO is making sure we are aligned with @elastic/kibana-platform — this is something we should all agree on, considering the registry pattern is so common in the KP.

@pgayvallet
Copy link
Contributor

We need to work on code-splitting app-arch code,

Would that be the prime goal / reason to switch to async registries ?

I feel like

Moving toward async registries would also give us longer term design flexibility, and could help solve some other problems unrelated to lazy loading (like making it easier to use core.getStartServices

would be a important part of the decision

Not clear how for example: to bundle all canvas specific expression function in one bundle.

Can I get more details on this need or any other that the async approach may make harder/impossible to achieve?

Fixing issues ad-hoc / Maybe such pragmatic approach is the way to go?

Code splitting is something we try to achieve on parts of the code that:

  • Can be asynchronously loaded, or can be adapted to be
  • Is depending on some heavy weight dependencies.

Application.mount is the perfect example here: all our applications are using react under the hood, meaning that code splitting the mount function get rid of react and associated ui dependencies from the plugin's root bundle and moving them to the new chunk instead, which is a significant perf improvement (both for initial load and to avoid loading unused code if a user was to never use the app)

For things like registries, I'm unsure code splitting is a real need, as least for the registry registration code itself, as implementation are (often) trivial and shouldn't depends on a lot of things (but this would need a service-per-service bundle analysis)

If I take the uiActions example: As Action.execute is already async, I think a splitting point could be easily introduced in the execute function, as it's very likely that's from here that the dependencies will be used, which make it an obvious potential split point imho.

Of course this would be an implementation detail of the plugins providing each individual action, as there is no way to ensure that the execute implementation is performing an async import, but this is the same issue as in the given example

uiActions.registerAction('ACTION_HELLO_WORLD', async () =>
      (await import('./hello_world_action_lazy')).createHelloWorldAction(async () => ({
        openModal: (await core.getStartServices())[0].overlays.openModal,
      }))
    );

could be

uiActions.registerAction('ACTION_HELLO_WORLD', () =>
      hello_world_action.createHelloWorldAction({
        openModal: core.getStartServices().then(([{overlays}]) => overlays.openModal),
      })
    );

hello_world_action.createHelloWorldAction = ({openModal}) => ({
  id,
  //...
  execute: async () => {
    const {execute} =  await import('./hello_world_action_execute');
   return execute(openModal);
  }
})

So imho it all go back to the

Moving toward async registries would also give us longer term design flexibility, and could help solve some other problems unrelated to lazy loading (like making it easier to use core.getStartServices

question

@ppisljar ppisljar added the technical debt Improvement of the software architecture and operational architecture label May 10, 2021
@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 labels Jun 21, 2021
@mattkime
Copy link
Contributor

mattkime commented Jul 1, 2021

Restarting this conversation as the data plugin has only gotten larger in the mean time.

For things like registries, I'm unsure code splitting is a real need, as least for the registry registration code itself, as implementation are (often) trivial and shouldn't depends on a lot of things (but this would need a service-per-service bundle analysis)

The benefit of splitting at the registry level is that it would be solution agnostic. For a single use case it really wouldn't matter but the data plugin alone has a couple of these.

Not clear how for example: to bundle all canvas specific expression function in one bundle.

I think this is something that we can design for.

@exalate-issue-sync exalate-issue-sync bot added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Jul 26, 2021
@Dosant Dosant changed the title [discuss] Code splitting AppArch registries? Async registries Aug 3, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort and removed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort labels Aug 3, 2021
@exalate-issue-sync exalate-issue-sync bot added loe:weeks and removed loe:large Large Level of Effort labels Sep 27, 2021
@ppisljar
Copy link
Member

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@ppisljar ppisljar closed this as not planned Won't fix, can't repro, duplicate, stale Aug 11, 2022
kibana-app-arch automation moved this from To triage to Done in current release Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Meta technical debt Improvement of the software architecture and operational architecture
Projects
kibana-app-arch
  
Done in current release
Development

No branches or pull requests

5 participants