STCOR-635 wrap pluggable in suspense #1213

zburke · 2022-06-09T18:15:20Z

Wrap Pluggable's return value in Suspense in order to avoid whole-tree
re-renders, since the whole tree from the components up to the nearest
<Suspense> is removed and replaced with its fallback. For us, this is
particularly a problem when a plugin is part of a dropdown menu that
uses calculations to set its position. Because the entire tree will be
unmounted, the calculations all return zero, resulting in incorrect
placement with zero offset from the let margin (STRWEB-53).

h/t @BogdanDenis and @mkuklis for figuring this all out.

Refs STCOR-635

@BogdanDenis

Wrap Pluggable's return value in Suspense in order to avoid whole-tree re-renders, since the whole tree from the components up to the nearest `<Suspense>` is removed and replaced with its fallback. For us, this is particularly a problem when a plugin is part of a dropdown menu that uses calculations to set its position. Because the entire tree will be unmounted, the calculations all return zero, resulting in incorrect placement with zero offset from the let margin (STRWEB-53). h/t @BogdanDenis and @mkuklis for figuring this all out. Refs STCOR-635

github-actions · 2022-06-09T18:16:19Z

Jest Unit Test Statistics

1 tests ±0 1 ✔️ ±0 1s ⏱️ ±0s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit e8ddb8a. ± Comparison against base commit 976a95c.

♻️ This comment has been updated with latest results.

github-actions · 2022-06-09T18:16:56Z

BigTest Unit Test Statistics

    1 files ±0     1 suites ±0 12s ⏱️ -1s
271 tests ±0 266 ✔️ ±0 5 💤 ±0 0 ❌ ±0
274 runs ±0 269 ✔️ ±0 5 💤 ±0 0 ❌ ±0

Results for commit e8ddb8a. ± Comparison against base commit 976a95c.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both.

      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_102_0_5005_61_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
Chrome_102_0_5005_61_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content

Chrome_102_0_5005_61_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_102_0_5005_61_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_102_0_5005_61_(Linux_x86_64).Forgot username form test ‑ Forgot username form test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation

♻️ This comment has been updated with latest results.

zburke · 2022-06-09T20:13:55Z

This is a nice idea, but it doesn't work. In a ui-inventory build with resolutions to this branch and to stripes-webpack with lazy turned back off for plugins, stripes isn't making its way through the hierarchy from StripesContext in stripesConnect:

Uncaught TypeError: Cannot read properties of undefined (reading 'connect')
    at new ConnectedComponent (stripesConnect.js:14:47)
    at constructClassInstance (react-dom.development.js:12716:1)
    ...

Interestingly, I also had to add dev-deps for stripes-form and stripes-final-form.

@mkuklis , @BogdanDenis , any ideas what could be going on?

BogdanDenis · 2022-06-10T07:58:51Z

@zburke hmm, I get a completely different error

Cannot access '__WEBPACK_DEFAULT_EXPORT__' before initialization

I'll investigate this more today

BogdanDenis · 2022-06-10T08:33:23Z

src/Pluggable.js

 import PropTypes from 'prop-types';
 import { modules } from 'stripes-config';
 import { withStripes } from './StripesContext';
-import { ModuleHierarchyProvider } from './components';
+import { LoadingView, ModuleHierarchyProvider } from './components';


@zburke I think <LoadingView> should come from @folio/stripes/components. This fixed my webpack error, but I didn't see what you reported in your comment

sonarqubecloud · 2022-06-10T16:04:03Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

BogdanDenis

Should use <Loading> instead - <LoadingView> will render a <Paneset> and this can potentially break pane resizing if there's a parent <Paneset>. Just spent a few hours debugging an issue that was caused by nested panesets, even though in theory they are valid.

I know it was I who suggested using <LoadingView> - so that's me fixing my mistake here

BogdanDenis · 2022-06-24T14:09:36Z

src/Pluggable.js

 import PropTypes from 'prop-types';
 import { modules } from 'stripes-config';
 import { withStripes } from './StripesContext';
 import { ModuleHierarchyProvider } from './components';
+import { LoadingView } from '@folio/stripes-components';


Suggested change

import { LoadingView } from '@folio/stripes-components';

import { Loading } from '@folio/stripes-components';

BogdanDenis · 2022-06-24T14:09:48Z

src/Pluggable.js

@@ -37,7 +38,9 @@ const Pluggable = (props) => {
  if (cachedPlugins.length) {
    return cachedPlugins.map(({ plugin, Child }) => (
      <ModuleHierarchyProvider module={plugin}>
-        <Child {...props} actAs="plugin" />
+        <Suspense fallback={<LoadingView />}>


Suggested change

<Suspense fallback={<LoadingView />}>

<Suspense fallback={<Loading />}>

BogdanDenis · 2022-06-24T14:09:58Z

src/Pluggable.js

@@ -47,7 +50,11 @@ const Pluggable = (props) => {
    // eslint-disable-next-line no-console
    console.error(`<Pluggable type="${props.type}"> has ${props.children.length} children, can only return one`);
  }
-  return props.children;
+  return (
+    <Suspense fallback={<LoadingView />}>


Suggested change

<Suspense fallback={<LoadingView />}>

<Suspense fallback={<Loading />}>

zburke · 2022-06-24T20:07:46Z

@BogdanDenis, we turned off lazy-loading of modules for Morning Glory in stripes-webpack in folio-org/stripes-webpack#69, which should turn <Suspense> into a do-nothing component because there are no separate chunks to wait for. IOW, we do need to <Loading> instead of <LoadingView> for the nested-paneset reason you identify, but it doesn't matter now, right? I guess I'm just making sure your stripes-webpack is up to date :)

BogdanDenis · 2022-06-25T15:07:31Z

@BogdanDenis, we turned off lazy-loading of modules for Morning Glory in stripes-webpack in folio-org/stripes-webpack#69, which should turn <Suspense> into a do-nothing component because there are no separate chunks to wait for. IOW, we do need to <Loading> instead of <LoadingView> for the nested-paneset reason you identify, but it doesn't matter now, right? I guess I'm just making sure your stripes-webpack is up to date :)

Yes, since lazy loading is turned off for now we don't need it. Just wanted to comment this so we won't miss this when we decide to turn lazy loading back on and merge this PR

Also, I spoke with @JohnC-80 about nested panesets, and he said that they should work, and do work for plugins and settings, so I was wrong about nested panesets causing problems
But still it would make more sense IMO to use <Loading> here

zburke requested review from mkuklis and BogdanDenis June 9, 2022 18:15

mkuklis approved these changes Jun 9, 2022

View reviewed changes

super helpful if you import the components you're using

0395552

BogdanDenis requested changes Jun 10, 2022

View reviewed changes

correctly import LoadingView

e8ddb8a

zburke marked this pull request as draft June 12, 2022 11:24

BogdanDenis approved these changes Jun 15, 2022

View reviewed changes

BogdanDenis requested changes Jun 24, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STCOR-635 wrap pluggable in suspense #1213

STCOR-635 wrap pluggable in suspense #1213

zburke commented Jun 9, 2022

github-actions bot commented Jun 9, 2022 •

edited

Loading

github-actions bot commented Jun 9, 2022 •

edited

Loading

zburke commented Jun 9, 2022

BogdanDenis commented Jun 10, 2022

BogdanDenis Jun 10, 2022

sonarqubecloud bot commented Jun 10, 2022

BogdanDenis left a comment

BogdanDenis Jun 24, 2022

BogdanDenis Jun 24, 2022

BogdanDenis Jun 24, 2022

zburke commented Jun 24, 2022

BogdanDenis commented Jun 25, 2022 •

edited

Loading

	import { LoadingView } from '@folio/stripes-components';
	import { Loading } from '@folio/stripes-components';

	<Suspense fallback={<LoadingView />}>
	<Suspense fallback={<Loading />}>

STCOR-635 wrap pluggable in suspense #1213

Are you sure you want to change the base?

STCOR-635 wrap pluggable in suspense #1213

Conversation

zburke commented Jun 9, 2022

github-actions bot commented Jun 9, 2022 • edited Loading

Jest Unit Test Statistics

github-actions bot commented Jun 9, 2022 • edited Loading

BigTest Unit Test Statistics

zburke commented Jun 9, 2022

BogdanDenis commented Jun 10, 2022

BogdanDenis Jun 10, 2022

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 10, 2022

BogdanDenis left a comment

Choose a reason for hiding this comment

BogdanDenis Jun 24, 2022

Choose a reason for hiding this comment

BogdanDenis Jun 24, 2022

Choose a reason for hiding this comment

BogdanDenis Jun 24, 2022

Choose a reason for hiding this comment

zburke commented Jun 24, 2022

BogdanDenis commented Jun 25, 2022 • edited Loading

github-actions bot commented Jun 9, 2022 •

edited

Loading

github-actions bot commented Jun 9, 2022 •

edited

Loading

BogdanDenis commented Jun 25, 2022 •

edited

Loading