Kibana: NP apps with custom `appRoute` fail to load due to missing legacy injected vars

Created on 10 Jan 2020  路  6Comments  路  Source: elastic/kibana

Chromeless apps with custom appRoute fail to load because of missing legacy injected vars required by non-related new platform plugins.

Steps to reproduce:

  1. Define the app:
core.application.register({
  appRoute: '/some',
  chromeless: true,
  id: 'some',
  title: 'some',
  mount({ element }) {
    function SomeComponent() {
      return <div>Some</div>;
    }
    render(<SomeComponent />, element);
    return () => {
      unmountComponentAtNode(element);
    };
  },
});
  1. Define the custom app route:
router.get(
  { path: '/some', validate: false },
  async (context, request, response) => {
    return response.ok({
        body: await context.core.rendering.render({ includeUserSettings: false }),
        headers: { 'content-security-policy': csp.header },
      });
  })
);
  1. Try to open http://localhost:5601/some in the browser.

Expected behavior:

View should load successfully.

Screenshots (if relevant):

Screenshot from 2020-01-10 17-12-28

Errors in browser console (if relevant):

Completely unrelated Newsfeed plugin causes fatal error due to missing injected var it depends on:

TypeError: "config is undefined"
    getApi api.ts:198
    fetchNewsfeed plugin.tsx:74
    start plugin.tsx:55
    _callee3$ plugin.ts:171
    tryCatch runtime.js:45
    invoke runtime.js:271
    method runtime.js:97
    asyncGeneratorStep plugin.ts:14
    _next plugin.ts:16
    _asyncToGenerator plugin.ts:16
    _asyncToGenerator plugin.ts:16
    start plugin.ts:187
    _callee2$ plugins_service.ts:249
    tryCatch runtime.js:45
    invoke runtime.js:271
    method runtime.js:97
    asyncGeneratorStep plugins_service.ts:12
    _next plugins_service.ts:14

/cc @eliperelman @legrego

New Platform Core blocker bug

Most helpful comment

Also, as already mentioned with @azasypkin on slack, I think we should really provide an helper API to avoid plugins manually registering their custom route path handler on every app using an appRoute.

Instead of

    router.get(
      {
        path: '/my-app-route',
        validate: false,
      },
      async (context, req, res) => {
        // + additional parameters for vars it seems
        const body = await context.core.rendering.render({ includeUserSettings: false }); 

        return res.ok({
          body,
          headers: {
            'content-security-policy': core.http.csp.header,
          },
        });
      }
    );

should we just add something like

core.http.registerCustomAppRoute('/my-app-route', { includeUserSettings, requireAuth })

As it seems 99% (maybe 100%?) of the usages will be the exact same code snippet, that we could automate inside core.

Also for futur-us: there is a typo in the security header that needs to be fixed for FT when we will add them:

https://github.com/elastic/kibana/blob/56041f03ada9730e701508dbc10032b69de9f218/src/plugins/testbed/server/index.ts#L95-L101

All 6 comments

Pinging @elastic/kibana-platform (Team:Platform)

So. The actual error cause is that when rendering a NP app, the injectedMetadata.vars is not populated, as this is an option that should be manually provided when calling

context.core.rendering.render({vars: [...]})

in the ui_render_mixin this is done using legacy API:

https://github.com/elastic/kibana/blob/357be5970dd6b67191f57fed7b32743045d166d2/src/legacy/ui/ui_render/ui_render_mixin.js#L185-L193

But when rendering NP app, it seems there is no way to actually provide these vars to the rendering service.

However the newsfeed NP plugin (and probably others) does access these vars, which results later in an NPE:

https://github.com/elastic/kibana/blob/a50dbefb62edf0be6b98110023afc32b04c06a7f/src/plugins/newsfeed/public/plugin.tsx#L55-L59

I thought we had functional tests covering this, but TIL they are only checking the response status, missing any actual error when displaying the page...

https://github.com/elastic/kibana/blob/054ec7036d01a1a2f817b0e50fa1b957f9562a5c/test/api_integration/apis/core/index.js#L37-L40

It seems we need a way to retrieve the legacy vars the same way ui_render_mixin does, however I'm not sure what the best way is (or if that's the actual solution)

@eliperelman @joshdover will need your insight on this one.

Also, as already mentioned with @azasypkin on slack, I think we should really provide an helper API to avoid plugins manually registering their custom route path handler on every app using an appRoute.

Instead of

    router.get(
      {
        path: '/my-app-route',
        validate: false,
      },
      async (context, req, res) => {
        // + additional parameters for vars it seems
        const body = await context.core.rendering.render({ includeUserSettings: false }); 

        return res.ok({
          body,
          headers: {
            'content-security-policy': core.http.csp.header,
          },
        });
      }
    );

should we just add something like

core.http.registerCustomAppRoute('/my-app-route', { includeUserSettings, requireAuth })

As it seems 99% (maybe 100%?) of the usages will be the exact same code snippet, that we could automate inside core.

Also for futur-us: there is a typo in the security header that needs to be fixed for FT when we will add them:

https://github.com/elastic/kibana/blob/56041f03ada9730e701508dbc10032b69de9f218/src/plugins/testbed/server/index.ts#L95-L101

It seems we need a way to retrieve the legacy vars the same way ui_render_mixin does, however I'm not sure what the best way is (or if that's the actual solution)

I think there's two options here:
1) Stop exposing the getInjected API to New Platform plugins. From what I understand there's only 2-3 plugins using this, and we plan to remove it anyways. It was only introduced as a stop-gap before https://github.com/elastic/kibana/pull/50641 was introduced.
2) Move all the legacy logic into the New Platform for getting the injected vars.

(1) is seems much more preferable as it's work that needs to be done anyways, and we won't throw any of it away like we would with the work required for (2).

I would like to think option 1 is the way to go, however the feature is currently broken, and going this path makes us depends on feature teams changes. I we decide to do this, would we handle the migration of getInjected-using code ourselves ?

For the newsfeed plugin, the migration is pretty trivial, just need to replace the exposed var to the new exposeToBrowser configuration.

Was this page helpful?
0 / 5 - 0 ratings