Cleanup spaces plugin #91976

jportner · 2021-02-19T06:37:49Z

Paying down technical debt on the Spaces plugin:

Cleans up imports, reordering/consolidating them and enforcing usage of the import type directive
Lazy-loads components, including reusable components that are exposed to consumers
Removes static export of SpaceAvatar component

jportner · 2021-02-20T15:42:18Z

@legrego I added lazy-loading for our reusable UI components in 27ed077 but it did not positively impact the page load bundle for the SOM and ML plugins. Perhaps we only get the performance benefit if we statically import components?

Snapshot of the Kibana Machine comment:

Async chunks

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

id before after diff

ml 6.4MB 6.4MB +594.0B

savedObjectsManagement 163.8KB 164.1KB +266.0B

total +860.0B

Page load bundle

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

id before after diff

spaces 274.7KB 275.5KB +771.0B

spacesOss 4.5KB 4.6KB +105.0B

total +876.0B

legrego · 2021-02-22T14:02:23Z

@jportner I haven't looked at the changes in this PR yet (so feel free to ignore this comment), but we might need to also incorporate the changes I started in #91018 in order to see page load improvements.

I'll try to circle back to this in the next day or so to take a closer look

jportner · 2021-02-22T14:50:24Z

So, here are the metrics for Phase 2.5 after we added the reusable UI components:

Async chunks

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

id before after diff

ml 6.4MB 6.3MB -14.4KB

savedObjectsManagement 163.7KB 164.1KB +356.0B

total -14.1KB

Page load bundle

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

id before after diff

ml 68.4KB 68.2KB -233.0B

spaces 231.3KB 274.9KB +43.6KB

spacesOss 3.5KB 4.5KB +1.0KB

total +44.5KB

The ML and SOM bundles never increased, but the Spaces bundle did. So I guess what you're saying here is that our goal is to decrease the Spaces page load bundle? That makes sense. I'll see if I can tinker with it a bit more.

legrego · 2021-02-22T15:02:22Z

The ML and SOM bundles never increased, but the Spaces bundle did. So I guess what you're saying here is that our goal is to decrease the Spaces page load bundle? That makes sense. I'll see if I can tinker with it a bit more.

Yes my goal was to decrease the Spaces page load bundle. I wasn't concerned about the ML/SOM bundles here

This is needed to use the import sorter

Sort order 1. External dependencies 2. Internal absolute dependencies (@kbn/..., src/...) 3. Internal relative dependencies Also cleaned up some relative dependencies in the process.

Adds ESLint rule to enforce this.

Splits out most components into separate chunks

An error was getting logged to the console because jsdom does not support window.location.reload() out of the box.

Rename it from 'CopySavedObjectsToSpaceFlyout' to 'CopyToSpaceFlyoutInternal'.

Rename it from 'SpaceAvatar' to 'SpaceAvatarInternal'

This was statically imported by the Security plugin so it required quite a bit of refactoring.

Reduces page load bundle by 29KB

Thought this would decrease the page load bundle size, but it only shaved off a few hundred bytes. At any rate, this change cleans up the code a bit, and will be needed when we eventually expose this as a reusable component for outside consumers.

jportner · 2021-02-22T22:39:43Z

OK, after a bunch of tweaks I've managed to get the page load bundle size cut down dramatically.

We had 29kb of transitive dependencies imported from @kbn/std, I was able to remove that too.

thomheymann · 2021-02-23T16:36:24Z

x-pack/plugins/ml/public/application/components/job_spaces_list/job_spaces_list.tsx

@@ -66,7 +66,9 @@ export const JobSpacesList: FC<Props> = ({ spacesApi, spaceIds, jobId, jobType,
    });
  }

-  const { SpaceList, ShareToSpaceFlyout } = spacesApi.ui.components;
+  const LazySpaceList = React.lazy(spacesApi.ui.components.getSpaceList);
+  const LazyShareToSpaceFlyout = React.lazy(spacesApi.ui.components.getShareToSpaceFlyout);


Dynamic imports using React.lazy should be created outside the render function, otherwise you're creating a new component with every render of JobSpacesList

There isn't really anything 'outside' of render in a FC 😅 . But useMemo should be used around the React.lazy declaration, yea (as it was properly done in saved_objects_table_page.tsx)

Highly depends on the answers on #91976 (comment) though.

I just pushed some changes.

Wherever I could do so (inside the Spaces plugin) I pulled out the lazy loading to the top level.
Otherwise, I memoized these inside of function components.

thomheymann · 2021-02-23T16:47:41Z

x-pack/plugins/ml/public/application/components/job_spaces_list/job_spaces_list.tsx

@@ -81,11 +83,11 @@ export const JobSpacesList: FC<Props> = ({ spacesApi, spaceIds, jobId, jobType,
  };

  return (
-    <>
+    <React.Suspense fallback={<EuiLoadingSpinner />}>


Suspense will throw an error if the lazy component cannot be loaded so you might want to wrap this in an error boundary and create a fallback.

This component is only rendered if the Spaces plugin is enabled and the Spaces API is available, and the getters to lazy load these components (getSpaceList, getShareToSpaceFlyout) are just promises to import the components that we are already guaranteed to have available. IMO we don't need to complicate this with an error boundary and fallback when we can rely on the Kibana platform's public contracts.

As discussed offline -- this is necessary if the user runs into any network errors at load time 😅 I'll add wrappers for these that display a toast message so the whole page doesn't blow up.

Edit: done in 032b5cb.

thomheymann · 2021-02-23T23:23:01Z

...gins/ml/public/application/management/jobs_list/components/jobs_list_page/jobs_list_page.tsx

+        spacesApi ? spacesApi.ui.components.getSpacesContext : getEmptyFunctionComponent
+      ),
+    [spacesApi]
+  );


I probably don't have enough context (lol, see what I did there?) so might be better to catch up tomorrow but it would be nice to be a bit clearer here in regards to naming.

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it? In that case it might be clearer to call the method getSpacesProvider and the return value SpacesProvider.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

Anyways, let's catch up tomorrow, just adding this as a reminder.

Yea, I share the same concerns. React context providers are usually static, and the value they provide mutates. I'm not sure to understand why we need an lazy accessor around a context provider?

it would be nice to be a bit clearer here in regards to naming.

I'm open to naming changes!

getSpacesContext to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it in React.lazy to create a lazy loaded component so it's just the Provider component by the looks of it?

Sort of. It's a "wrapper" component that includes the context provider. We need this because when this wrapper is mounted, it makes a series of API calls and caches the results, then makes those available to its children via the context provider.

The function to obtain this wrapper within the Spaces plugin is called getSpacesContextWrapper, but the one that is exposed to consumers in the public contract is called getSpacesContext. We could rename the latter to something like getSpacesContextProviderWrapper but I don't know what value that really adds for consumers, it's an implementation detail IMO. They don't need to consume the context provider, they just need to know that they have to render the flyout, space list, etc. as a descendant of this wrapper.

Having said that, Context (returned by createContext) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.

I don't think so, we need the wrapper.

I'm not sure to understand why we need an lazy accessor around a context provider?

Because it's not just a context provider, it's the wrapper that also fetches data. Also, to flip that question around: why not provide a lazy accessor? All of the other components have lazy accessors. It seems this approach is more consistent.

Caught up with Joe and I can't think of a good alternative to avoid the architectural complexity with the approach here.

To fulfil the brief, we need a way to make the spaces state available globally, so that we can provide drop-in components for developers that share that state. However, we're not able to render the context provider at the root level of the application ourselves (since spaces is a plugin) and we can't provide static exports (due to the restriction imposed by the platform), hence the indirection using dynamically created and memoized context providers, that require developers to consume them using the spaces plugin contract rather than static imports like EUI.

I'm guessing this is a bit of an edge case and other plugin developers don't have the same requirements to share core functionality like spaces does. However, I would argue that certain things like i18n, routing, session information, authenticated user information and in this particular case spaces are global concerns that should be available throughout the application. Usually these are managed at the root level in React applications so that any child component has access via hooks and context but this isn't really possible with the way we've currently modelled spaces as a plugin.

Happy to leave this as is for now but I am slightly worried about the complexity and maintainability of the current solution.

Components exposed in the public contract are now wrapped and automatically lazy-loaded. Consumers no longer have to handle this.

If we have an exception where we do not use an error boundary, I added a comment explaining why. I also standardized our usages of `lazy` and `Suspense` by destructuring the React import.

thomheymann

Looks good, couple of comments related to your usage of hooks.

...ublic/management/roles/edit_role/privileges/kibana/privilege_summary/space_column_header.tsx

x-pack/plugins/spaces/public/copy_saved_objects_to_space/copy_saved_objects_to_space_action.tsx

...k/plugins/spaces/public/share_saved_objects_to_space/share_saved_objects_to_space_action.tsx

...k/plugins/spaces/public/share_saved_objects_to_space/share_saved_objects_to_space_column.tsx

thomheymann · 2021-02-25T19:30:02Z

x-pack/plugins/spaces/public/ui_api/lazy_wrapper.tsx

+    getStartServices().then(([coreStart]) => {
+      setNotifications(coreStart.notifications);
+    });
+  });


Again, the side effect here will run at every render unless you add a dependency array and you also need to ensure the component is still mounted when modifying state asynchronously.

What you could do so you don't have to manage all that yourself is use the useAsync hook from react-use module:

const { value: { notifications } } = useAsync(getStartServices); // `value` is the `coreStart` object returned by `getStartServices` return <SuspenseErrorBoundary notifications={notifications} />

https://streamich.github.io/react-use/?path=/story/side-effects-useasync--docs

Good idea, thanks!
Destructuring makes this a bit funky, but this is what I did to make it work:

const { value: startServices = [{ notifications: undefined }] } = useAsync(getStartServices); const [{ notifications }] = startServices;

x-pack/plugins/spaces/public/ui_api/lazy_wrapper.tsx

x-pack/plugins/spaces/public/suspense_error_boundary/suspense_error_boundary.tsx

jportner · 2021-03-01T10:57:31Z

@elasticmachine merge upstream

thomheymann

Looks gooooood!!!

thomheymann · 2021-03-01T11:03:14Z

x-pack/plugins/spaces/public/ui_api/lazy_wrapper.tsx

-      setNotifications(coreStart.notifications);
-    });
-  });
+  const { value: startServices = [{ notifications: undefined }] } = useAsync(getStartServices);


For next time: An empty object should be enough here:

const { value: startServices = [{}] } = useAsync(getStartServices);

Ah, I did this because there's no type overlap between CoreStart and {}, so tsc complains about that. Using { notifications: undefined } allows for type inferencing. Now that you mention it though I suppose this would work

const { value: startServices = [{} as CoreStart] } = useAsync(getStartServices);

I think it's a bit better to have explicit types but we've got a big mix of inferences + explicit typing everywhere 🙈

kibanamachine · 2021-03-01T12:56:42Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`spaces`	270	259	-11

Async chunks

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

id	before	after	diff
`ml`	6.4MB	6.4MB	+229.0B
`savedObjectsManagement`	163.8KB	163.8KB	-20.0B
`security`	723.8KB	724.3KB	+484.0B
`spaces`	41.5KB	309.3KB	⚠️ +267.8KB
total			+268.5KB

Page load bundle

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

id	before	after	diff
`security`	130.9KB	131.0KB	+104.0B
`spaces`	274.7KB	44.6KB	-230.1KB
`spacesOss`	4.5KB	4.7KB	+212.0B
total			-229.8KB

Unknown metric groups

async chunk count

id	before	after	diff
`spaces`	1	10	+9

History

💚 Build #109594 succeeded 38fb012
💔 Build #109558 failed 2612620
💔 Build #109527 failed b792353
💔 Build #109519 failed bebf3b2
💚 Build #108613 succeeded b8d93b0

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

kibanamachine · 2021-03-01T12:59:20Z

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 91976

* Cleanup spaces plugin (#91976) # Conflicts: # x-pack/plugins/security/public/management/roles/roles_management_app.tsx * Make the linter happy Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

… ilm/rollup-v2-action * 'ilm/rollup-v2-action' of github.com:elastic/kibana: [Security Solution][Case][Bug] Only add rule object for alert comments (#92977) [Security Solution][Case] Show the current connector name in case view (#93018) [Security Solution] Remove unused mock data (#92357) Adds mapping to the signals for the indicator rules that were missing (#92928) skip flaky suite (#85208) Cleanup spaces plugin (#91976) Control round and decimal places in Gauge Visualization when using aggregate functions like average (#91293) Added alerting ui mock for jest test (#92604) Remove "beta" label from URL Drilldown as it is now GA (#92859)

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Feb 19, 2021

jportner force-pushed the cleanup-spaces-plugin branch 3 times, most recently from 83015b5 to 27ed077 Compare February 19, 2021 22:20

jportner added 16 commits February 22, 2021 17:26

Convert relative imports to absolute imports

fad2f42

Convert more relative imports to absolute imports

388206d

Temporarily remove import type directive

dc49b70

This is needed to use the import sorter

Sort imports

25a9066

Sort order 1. External dependencies 2. Internal absolute dependencies (@kbn/..., src/...) 3. Internal relative dependencies Also cleaned up some relative dependencies in the process.

Apply import type directive

97cc9bb

Adds ESLint rule to enforce this.

Add lazy loading of reusable Spaces UI components

4ad077f

Reduce bundle size

f022c4c

Splits out most components into separate chunks

Fix jest test error

59a921c

An error was getting logged to the console because jsdom does not support window.location.reload() out of the box.

Refactor CTS flyout

6284843

Rename it from 'CopySavedObjectsToSpaceFlyout' to 'CopyToSpaceFlyoutInternal'.

Move summarizeCopyResult

0872427

Split CTS flyout into a separate chunk

9341c0c

Lazy load NavControlPopover

0080228

Refactor SpaceAvatar

f3126fd

Rename it from 'SpaceAvatar' to 'SpaceAvatarInternal'

Lazy load SpaceAvatar

9b6cf54

This was statically imported by the Security plugin so it required quite a bit of refactoring.

Remove dependency on @kbn/std package

c922c54

Reduces page load bundle by 29KB

Change CTS flyout to use SpacesContext

b8d93b0

Thought this would decrease the page load bundle size, but it only shaved off a few hundred bytes. At any rate, this change cleans up the code a bit, and will be needed when we eventually expose this as a reusable component for outside consumers.

jportner force-pushed the cleanup-spaces-plugin branch from 27ed077 to b8d93b0 Compare February 22, 2021 22:39

jportner marked this pull request as ready for review February 23, 2021 03:56

jportner requested a review from a team as a code owner February 23, 2021 03:56

thomheymann reviewed Feb 23, 2021

View reviewed changes

pgayvallet approved these changes Feb 25, 2021

View reviewed changes

jportner added 5 commits February 25, 2021 11:00

Merge branch 'master' into pr/jportner/91976

6242d1a

Abstract out lazy-loading from reusable components

a21ca9a

Components exposed in the public contract are now wrapped and automatically lazy-loaded. Consumers no longer have to handle this.

Use error boundaries when lazy-loading components

032b5cb

If we have an exception where we do not use an error boundary, I added a comment explaining why. I also standardized our usages of `lazy` and `Suspense` by destructuring the React import.

Rename getSpacesContext to getSpacesContextProvider

bebf3b2

Fix type check for unit test

b792353

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 25, 2021

jportner enabled auto-merge (squash) February 25, 2021 18:38

jportner added 2 commits February 25, 2021 13:59

Fix i18n error

181ddcb

Remove unused parameter

2612620

thomheymann requested changes Feb 25, 2021

View reviewed changes

jportner added 3 commits February 25, 2021 14:56

Merge branch 'master' into pr/jportner/91976

1b6554b

PR review feedback

1b6c5e5

Fix jest unit tests

38fb012

Merge branch 'master' into cleanup-spaces-plugin

99ab44c

thomheymann approved these changes Mar 1, 2021

View reviewed changes

jportner merged commit 8710a81 into elastic:master Mar 1, 2021

This was referenced Mar 1, 2021

[7.x] Cleanup spaces plugin (#91976) #93032

Merged

Cleanup Security plugin imports #93056

Merged

jportner deleted the cleanup-spaces-plugin branch March 1, 2021 16:16

jportner added the backported label Mar 1, 2021

legrego mentioned this pull request Mar 2, 2021

Async loading of spaces assets #91018

Closed

jportner mentioned this pull request Mar 4, 2021

ESLint rules for Spaces and Security #93644

Merged

legrego mentioned this pull request May 14, 2021

[Workplace Search] Add Account Settings page imported from Security plugin #99791

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup spaces plugin #91976

Cleanup spaces plugin #91976

jportner commented Feb 19, 2021 •

edited

Loading

jportner commented Feb 20, 2021

Async chunks

Page load bundle

legrego commented Feb 22, 2021

jportner commented Feb 22, 2021

Async chunks

Page load bundle

legrego commented Feb 22, 2021

jportner commented Feb 22, 2021

thomheymann Feb 23, 2021

pgayvallet Feb 24, 2021 •

edited

Loading

jportner Feb 25, 2021 •

edited

Loading

thomheymann Feb 23, 2021

jportner Feb 24, 2021

jportner Feb 24, 2021 •

edited

Loading

thomheymann Feb 23, 2021

pgayvallet Feb 24, 2021

jportner Feb 24, 2021

thomheymann Feb 24, 2021

thomheymann left a comment

thomheymann Feb 25, 2021

jportner Feb 25, 2021

jportner commented Mar 1, 2021

thomheymann left a comment

thomheymann Mar 1, 2021

jportner Mar 1, 2021

kibanamachine commented Mar 1, 2021

async chunk count

kibanamachine commented Mar 1, 2021

Cleanup spaces plugin #91976

Cleanup spaces plugin #91976

Conversation

jportner commented Feb 19, 2021 • edited Loading

jportner commented Feb 20, 2021

Async chunks

Page load bundle

legrego commented Feb 22, 2021

jportner commented Feb 22, 2021

Async chunks

Page load bundle

legrego commented Feb 22, 2021

jportner commented Feb 22, 2021

Choose a reason for hiding this comment

pgayvallet Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

jportner Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner Feb 24, 2021 • edited Loading

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

thomheymann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jportner commented Mar 1, 2021

thomheymann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 1, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

kibanamachine commented Mar 1, 2021

💔 Backport failed

jportner commented Feb 19, 2021 •

edited

Loading

pgayvallet Feb 24, 2021 •

edited

Loading

jportner Feb 25, 2021 •

edited

Loading

jportner Feb 24, 2021 •

edited

Loading