[Canvas] Remove Angular and unnecessary reporting config from Canvas #54050

poffdeluxe · 2020-01-06T20:03:07Z

Summary

Removed Angular from the Canvas application
- Removed the controller as well as state setup
Added a Canvas "app"
Removed workpad export disabled panel since reporting no longer has the option to use phantom for generating PDFs
Modified the store setup the be more NP compliant by trying to usage of injected variables
- Removed kbnVersion from frontend state since we don't appear to use it anywhere I could find

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2020-01-06T20:03:09Z

Pinging @elastic/kibana-canvas (Team:Canvas)

x-pack/legacy/plugins/canvas/public/plugin.tsx

poffdeluxe · 2020-01-06T20:05:33Z

x-pack/legacy/plugins/canvas/public/plugin.tsx

    initClipboard(plugins.__LEGACY.storage);
    initLoadingIndicator(core.http.addLoadingCountSource);

-    const CanvasRootController = CanvasRootControllerFactory(core, plugins);
-    plugins.__LEGACY.setRootController('canvas', CanvasRootController);
+    core.chrome.setBadge(


this was just moved from the controller. Definitely need to test read only

poffdeluxe · 2020-01-06T20:07:52Z

x-pack/legacy/plugins/canvas/types/state.ts

@@ -32,7 +32,6 @@ export interface AppState {
 }

 interface StoreAppState {
-  kbnVersion: string;


I couldn't find any usage of the kbnVersion in our frontend code so I'm removing it

poffdeluxe · 2020-01-06T21:17:02Z

x-pack/legacy/plugins/canvas/public/applications/canvas.tsx

+    element
+  );
+  return () => ReactDOM.unmountComponentAtNode(element);
+};


This was created based on the new conventions laid out here: https://github.com/elastic/kibana/blob/0dac43516e0690486d13071063cb089bce96ee11/src/core/CONVENTIONS.md#applications

The thing is, it's kinda weird to have applications at the same level as our apps directory (which houses home, workpad, and export)

Could this just live in the root of public as just application.tsx or something like that?

poffdeluxe · 2020-01-07T20:55:36Z

x-pack/legacy/plugins/canvas/public/plugin.tsx

-    const CanvasRootController = CanvasRootControllerFactory(core, plugins);
-    plugins.__LEGACY.setRootController('canvas', CanvasRootController);
+    core.chrome.setBadge(
+      core.application.capabilities.canvas && core.application.capabilities.canvas.save


Originally this was checking for the existence just of core.application.capabilities.canvas.save but I was running into a test error where .canvas wasn't defined on capabilities. I fixed it by adding this check. If we want to fix this by doing a mock somewhere for the test instead, I can do that

rudolf

The way Core API's are consumed lgtm 👍

rudolf · 2020-01-09T12:08:44Z

x-pack/legacy/plugins/canvas/public/applications/canvas.tsx

+  canvasStore: Store
+) => {
+  ReactDOM.render(
+    <KibanaContextProvider services={{ ...coreStart, ...plugins }}>


nit: The KibanaContextProvider uses react context to provide enable deeply nested react components to directly consume these services. I didn't look closely at the Canvas code, but since you seem to be using Redux, you probably won't need this context and would instead consume Core API's from reducers.

@rudolf We are using useSetting and other hooks from kibana-react and I thought those required setting a context like this, or is there a way to do that without requiring a context like this?

If you're using the hooks from kibana-react then this is correct.

rudolf · 2020-01-09T12:11:37Z

Caveat: reportingBrowserType is a config var coming from the reporting side of things. As far as I can tell, NP doesn't have a great way to retrieve cross-plugin config at the moment so I kept it using injected variables for now until we can replace it with something better

NP doesn't / won't allow cross-plugin config reading, so the reporting plugin will have to expose any of it's config values that are meant to be consumed by other plugins from it's setup or start API's.

crob611

Looks good, just a couple of nits.

If we could remove the bulk of the stuff on the server side that is injecting the vars, that would be great too.

crob611 · 2020-01-17T16:31:24Z

x-pack/legacy/plugins/canvas/public/applications/canvas.tsx

+    element
+  );
+  return () => ReactDOM.unmountComponentAtNode(element);
+};


Could this just live in the root of public as just application.tsx or something like that?

crob611 · 2020-01-17T16:33:05Z

x-pack/legacy/plugins/canvas/public/applications/canvas.tsx

+  canvasStore: Store
+) => {
+  ReactDOM.render(
+    <KibanaContextProvider services={{ ...coreStart, ...plugins }}>


@rudolf We are using useSetting and other hooks from kibana-react and I thought those required setting a context like this, or is there a way to do that without requiring a context like this?

crob611 · 2020-01-17T20:47:25Z

x-pack/legacy/plugins/canvas/public/store.ts

+  const initialState = getInitialState();
+
+  const basePath = core.http.basePath.get();
+  const reportingBrowserType = core.injectedMetadata.getInjectedVar('reportingBrowserType');


Did we decide that injectedVars are going away? If so, let's shim out the reporting plugin in our dependency and pass that in here instead.

crob611 · 2020-01-21T19:25:13Z

x-pack/legacy/plugins/canvas/public/plugin.tsx

@@ -43,9 +43,6 @@ export interface CanvasStartDeps {
    setRootController: Chrome['setRootController'];


Can setRootController also be removed? Think we're just using the application.register now?

tsullivan · 2020-01-23T23:06:31Z

x-pack/legacy/plugins/canvas/public/legacy.ts

+  reporting:
+    npSetup.core && npSetup.core.injectedMetadata.getInjectedVar('reportingBrowserType')
+      ? {}
+      : undefined,


The reason that Canvas had to check the Reporting browser type was because, if it was configured to phantom, the, the PDF would fail. Originally it was important to show that the configuration had to change and make the browser type become chromium

In Kibana 7.x we dropped support for phantom, and now chromium is the only valid option for the type setting! The setting will probably go away completely in 8.0.

Letting you know this, because you can remove lots of code in Canvas, since the browser type check isn't needed! 🍾

tsullivan · 2020-01-23T23:07:43Z

x-pack/legacy/plugins/canvas/server/plugin.ts


    core.injectUiAppVars('canvas', async () => {
      const config = core.getServerConfig();
-      const basePath = config.get('server.basePath');
      const reportingBrowserType = (() => {


No need to worry about this, per https://github.com/elastic/kibana/pull/54050/files#r370402125

Leaving this as-is would complicate the Reporting team from upgrading to the NP config service anyway 😄

tsullivan · 2020-01-23T23:23:29Z

You could probably get rid of I think you can get rid of x-pack/legacy/plugins/canvas/public/components/workpad_header/workpad_export/disabled_panel.tsx if you wanted.

poffdeluxe · 2020-01-27T15:50:40Z

@elasticmachine merge upstream

kibanamachine · 2020-01-27T17:18:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dbbef47

History

💚 Build #22536 succeeded 37c6eb5
💔 Build #22525 failed 4a89637
💔 Build #22505 failed fad8e85
💔 Build #22476 failed ad15369
💚 Build #22148 succeeded ba3bb296af9abef668c14ecbed030b3cf2937f41

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

…ve-out-legacy * 'master' of github.com:elastic/kibana: (187 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ... # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx # src/legacy/ui/public/vis/editors/default/default_editor.tsx # src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/console/public/application/components/split_panel/components/resizer.tsx # src/plugins/console/public/application/components/split_panel/containers/panel.tsx # src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx # src/plugins/console/public/application/components/split_panel/context.tsx # src/plugins/console/public/application/components/split_panel/index.ts # src/plugins/console/public/application/components/split_panel/registry.ts # src/plugins/console/public/application/components/split_panel/split_panel.test.tsx # src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/kibana_react/public/split_panel/containers/panel.tsx # src/plugins/kibana_react/public/split_panel/context.tsx # src/plugins/kibana_react/public/split_panel/index.ts # src/plugins/kibana_react/public/split_panel/split_panel.test.tsx

* master: (77 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ...

…54050) (#56067) * Remove Angular from Canvas * Remove reporting config behavior from Canvas since it's no longer needed Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

poffdeluxe requested a review from crob611 January 6, 2020 20:03

poffdeluxe commented Jan 6, 2020

View reviewed changes

x-pack/legacy/plugins/canvas/public/plugin.tsx Outdated Show resolved Hide resolved

poffdeluxe commented Jan 6, 2020

View reviewed changes

poffdeluxe added v7.6.0 and removed v7.7.0 labels Jan 7, 2020

poffdeluxe commented Jan 7, 2020

View reviewed changes

poffdeluxe marked this pull request as ready for review January 7, 2020 22:13

poffdeluxe requested a review from a team as a code owner January 7, 2020 22:13

poffdeluxe added the review label Jan 8, 2020

rudolf reviewed Jan 9, 2020

View reviewed changes

poffdeluxe added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

crob611 approved these changes Jan 17, 2020

View reviewed changes

poffdeluxe force-pushed the canvas-remove-angular branch from 20e8542 to 55e43f2 Compare January 21, 2020 19:19

crob611 reviewed Jan 21, 2020

View reviewed changes

tsullivan reviewed Jan 23, 2020

View reviewed changes

poffdeluxe added 9 commits January 24, 2020 14:14

Remove Angular from Canvas

7d940a1

Updates i18n string names

6b65d75

Updates path

88c74e9

Another fix

cd25d91

Modifying save check for canvas plugin

23b28b5

PR feedback and adjusting reporting config

7de5b89

Adding clarifying comment and removing root controller

7f97c7f

test fix

25b4646

Remove reporting config behavior from Canvas since it's no longer needed

ad15369

poffdeluxe force-pushed the canvas-remove-angular branch from ba3bb29 to ad15369 Compare January 24, 2020 20:22

Remove disabled panel tests

fad8e85

poffdeluxe changed the title ~~[Canvas] Remove Angular from Canvas~~ [Canvas] Remove Angular and unnecessary reporting config from Canvas Jan 24, 2020

poffdeluxe added 2 commits January 24, 2020 16:43

Fix workpad export snapshot

4a89637

Update unused i18n

37c6eb5

Merge branch 'master' into canvas-remove-angular

dbbef47

poffdeluxe merged commit be9d9c2 into elastic:master Jan 27, 2020

poffdeluxe deleted the canvas-remove-angular branch January 27, 2020 21:01

poffdeluxe mentioned this pull request Jan 27, 2020

[7.x] [Canvas] Remove Angular and unnecessary reporting config from Canvas (#54050) #56067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Remove Angular and unnecessary reporting config from Canvas #54050

[Canvas] Remove Angular and unnecessary reporting config from Canvas #54050

poffdeluxe commented Jan 6, 2020 •

edited

Loading

elasticmachine commented Jan 6, 2020

poffdeluxe Jan 6, 2020

poffdeluxe Jan 6, 2020

poffdeluxe Jan 6, 2020

crob611 Jan 17, 2020

poffdeluxe Jan 7, 2020

rudolf left a comment

rudolf Jan 9, 2020

crob611 Jan 17, 2020

rudolf Jan 27, 2020

rudolf commented Jan 9, 2020

crob611 left a comment

crob611 Jan 17, 2020

crob611 Jan 17, 2020

crob611 Jan 17, 2020

crob611 Jan 21, 2020

tsullivan Jan 23, 2020

tsullivan Jan 23, 2020

tsullivan commented Jan 23, 2020

poffdeluxe commented Jan 27, 2020

kibanamachine commented Jan 27, 2020

		@@ -43,9 +43,6 @@ export interface CanvasStartDeps {
		setRootController: Chrome['setRootController'];

[Canvas] Remove Angular and unnecessary reporting config from Canvas #54050

[Canvas] Remove Angular and unnecessary reporting config from Canvas #54050

Conversation

poffdeluxe commented Jan 6, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf commented Jan 9, 2020

crob611 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Jan 23, 2020

poffdeluxe commented Jan 27, 2020

kibanamachine commented Jan 27, 2020

💚 Build Succeeded

History

poffdeluxe commented Jan 6, 2020 •

edited

Loading