8423 Modular plugins #8457

alexander-fedorenko · 2022-08-02T11:02:17Z

Description

This PR introduces support of module plugins.
See #8423 and https://github.com/geosolutions-it/MapStore2/wiki/%5BProposal%5D-MapStore-Modular-Plugins
for more details.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

Issue

What is the current behavior?
#8423

What is the new behavior?
MapStore supports module plugins. Plugin implementation is loaded dynamically on demand.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

Yes, and I documented them in migration notes
No

Other useful information

- ReducerManager, updated StandardStore to use it.

Passing lazy plugins into PluginsContainer component.

Share plugin definition fix.

Re-use plugins cache whenever hook is used by a new PluginsContainer instance. Cleanup, making user manager page works again.

…tation.

- Moving Module plugin utils into separate file - Naming conventions: Lazy loaded plugins are "Module plugins"

MapViewerCmp was dispatching INIT_MAP action unconditionally on component mount. This was happening prior to the dynamically imported epics were registered. Therefore, some epics missed INIT_MAP action and were not running when they should. This behavior is slightly changed, MapViewerCmp will instead set a state variable holding a function that needs to run whenever plugins are mounted. State value is passed down to the withModulePlugins wrapper of PluginsContainer and function is running whenever PluginsContainer is ready to be rendered.

…ded whenever MapsPlugin is converted into lazy one. The problem was caused by loadMaps action being dispatched on page mount, again, while dynamically loaded epics are not registered yet. With the change, action will be dispatched by PluginsContainer, when it is mounted on the page. And this will happen after module plugins are successfully loaded.

… change =

allyoucanmap

The implementation seems good, most of the comments are related to naming, questions or typos.
Before to proceed with the issue related with specific plugin I would suggest to complete the support for extension, once we have also that support I'll do another round of test.

We should also verify the final bundles sizes with the webpack analyzer after this initial work

web/client/components/app/main.jsx

allyoucanmap · 2022-08-04T14:35:13Z

web/client/components/app/withExtensions.js

@@ -101,10 +101,10 @@ function withExtensions(AppComponent) {
        };

        render() {
-            const { plugins, requires } = this.props.pluginsDef;
+            const { plugins, ...rest } = this.props.pluginsDef;


Suggested change

const { plugins, ...rest } = this.props.pluginsDef;

const { plugins, requires } = this.props.pluginsDef;

web/client/components/app/withExtensions.js

allyoucanmap · 2022-08-04T14:41:40Z

web/client/components/plugins/PluginsContainer.jsx

+export default compose(
+    connect((state) => ({
+        mode: urlQuery.mode || (urlQuery.mobile || state.browser && state.browser.mobile ? 'mobile' : 'desktop')
+    })),
+    withModulePlugins()
+)(PluginsContainer);


We should add connect or redux in components folder. So please remove all the connect.
Could we also export plugin without HOC?

export const PluginsContainer; export default withModulePlugins(PluginsContainer);

Did you mean we should not add? I removed connect and moved it instead into HOC.
Export with HOC allows to simplify usage of resulting component on a list of pages. In a few places import + usage of connect is replaced now with only import; another places doing connect to a fewer list of state values. It also makes code a bit more error-proof.

Why do we need the connect also in the HOC?
We are already connecting everything in the page, both hook, HOC and component should not have connect

As agreed in a call, applied solution with having separate container "ModulePluginsContainer" specifically for MapStore pages. This container is wrapped by a "withModulePlugins" HOC to support processing of module plugins.

allyoucanmap · 2022-08-04T15:18:57Z

web/client/containers/MapViewer.jsx

-    }),
-    monitoredState: getMonitoredState(state, ConfigUtils.getConfigProp('monitorState'))
-}))(require('../components/plugins/PluginsContainer').default);
+const PluginsContainer = compose(


We can remove compose now

Sure, removed

allyoucanmap · 2022-08-04T15:26:21Z

web/client/utils/StateUtils.js

+// const fetchEpic = (storeName = PERSISTED_STORE_NAME, name = 'rootEpic') => {
+//     return ConfigUtils.getConfigProp(storeName + '.' + name) || {};
+// };


Could we remove this including the relative doc?

allyoucanmap · 2022-08-04T15:29:58Z

web/client/utils/StateUtils.js

+        addEpics: (key, epicsList) => {
+            muteState[key] = new Subject();
+            const isolatedEpics = isolateEpics(epicsList, muteState[key].asObservable());
+            // wrapEpics(isolateEpics(epicsList, muteState[key].asObservable())).forEach(epic => epic$.next(epic));


Is this code still needed?

Removed commented line

web/client/utils/StateUtils.js

allyoucanmap · 2022-08-04T15:37:31Z

web/client/product/components/viewer/MapViewerCmp.jsx

+        this.setState({ initMap: () => {
+            this.updateMap(id, contextId);
+            this.setState({initMap: null});
+        }});


Based on the pvt discussion we should do the follow:

revert this fix for the moment with all related code around

plugin that has still issue because of this should be kept static for the moment

once we have the list with all the problematic plugins we can evaluate how to proceed

Rolled back, list of plugins is to be added in the comment down below in PR.

allyoucanmap · 2022-08-04T15:41:08Z

web/client/product/plugins.js

@@ -6,138 +6,143 @@
 * LICENSE file in the root directory of this source tree.
 */

+import {toModulePlugin} from "../utils/ModulePluginsUtils";
+


Please replace require with import for static plugins

…eing loaded whenever MapsPlugin is converted into lazy one. The problem was caused by loadMaps action being dispatched on page mount, again, while dynamically loaded epics are not registered yet. With the change, action will be dispatched by PluginsContainer, when it is mounted on the page. And this will happen after module plugins are successfully loaded." This reverts commit c50f557

… properly. MapViewerCmp was dispatching INIT_MAP action unconditionally on component mount. This was happening prior to the dynamically imported epics were registered. Therefore, some epics missed INIT_MAP action and were not running when they should. This behavior is slightly changed, MapViewerCmp will instead set a state variable holding a function that needs to run whenever plugins are mounted. State value is passed down to the withModulePlugins wrapper of PluginsContainer and function is running whenever PluginsContainer is ready to be rendered." This reverts commit 784c483

…Included common values from state into definition so usage of PluginsContainer on page components become shorter and simpler.

- Fixes for test coverage.

alexander-fedorenko · 2022-08-10T12:32:11Z

Bundle analyzer after recent updates

Static plugins included

Parsed

Gzipped

Static plugins excluded

Parsed

Gzipped

allyoucanmap · 2022-08-10T16:25:39Z

@tdipisa @alexander-fedorenko

Added dynamic import for ThematicLayerPlugin and StyleEditorPlugin. I can't test them locally, but they have no epics relying on actions triggered before PluginsContainer get rendered, so both of them should be fine.

We will test this once merged

Applied corrections to the code that mute or unmute epics. Now they will normalize plugins names just like it happens in plugins cache.

ok, good

Configuration for Catalog, Measurement, Annotations is not loaded properly unless reducers are added into list of appReducers. In case of dynamic import default reducer state overwrite configuration passed upon store creation. So either they have to be in appReducers, or default state in reducer should be taken somewhere outside using utility function.

I think we can keep them as appReducers, they haven't effect on app size,
checking the Analyzer results latest branch update is 6.5 MB vs 6.75 MB previous analysis (not Gzipped)

We will open a separate issue to solve the configuration for these plugins

We can use dynamic import, checking with @allyoucanmap if suggested approach is sufficient.

We will open a separate issue to investigate properly on the css-tree import

Test coverage is extended, withModulePlugins & useModulePlugins are covered.

Three updates are on the way:

Documentation about transforming extension into module extension.

Guideline to update from augmentStore to storeManager use.

Making sub-stream mutable when outer epic is muted.

good, waiting for this latest update for the final review, thanks

tdipisa · 2022-08-10T16:32:14Z

Wonderful @allyoucanmap and @alexander-fedorenko. @alexander-fedorenko I'm looking forward to receiving the last update soon to start the testing phase in DEV with @ElenaGallo.

- Restored augmentStore but marked it as deprecated

alexander-fedorenko · 2022-08-11T07:33:22Z

@tdipisa @allyoucanmap Documentation update is committed. There are two new documentation chapters.
I also recovered augmentStore in StateUtils but marked it as deprecated in favor of storeManager usage.

tdipisa · 2022-08-11T09:35:50Z

@ElenaGallo let's schedule a call to discuss about this and schedule the testing phase in DEV.

tdipisa · 2022-08-12T12:57:47Z

@ElenaGallo after the test phase the update provided through this PR will need to be backported to 2022.02.xx to be included in the release 2022.02.00

offtherailz · 2022-08-16T09:29:10Z

Bundle analyzer after recent updates

Static plugins included

Parsed

Gzipped

Static plugins excluded

Parsed

Gzipped

Just a note about this:

Most of the libs in the main package can be optimized more in the future :
- QuillJS should be replaced with React-draft-wysiwyg (custom)
- React-draft-wysiwyg (and so draft.js) should be loaded with React.lazy in every instance. The same for codemirror. This may allow to reduce even more the size of the main JS.
- pdf.js should be already loaded in a lazy way. For some reason is included in the main package again. Same for shp.js or zip.js. Anyway they look a lot small, so we may need to investigate more about these libs parts.
From lighthouse analysis, to increase the score we may need to imrpove caching of JS (expecially for the main JS) introducing the chunk hash in the name (e.g. mapstore2.js --> mapstore2-aaasddgfdgt43.js) Anyway the performance looks to be 2.5 - 3 times better.

Summarizing, pdf.js is the only thing that looks strange to me to see in the main package. The rest of the things can be improved one by one later.

(cherry picked from commit 4005b4e)

alexander-fedorenko added 4 commits July 20, 2022 17:06

geosolutions-it#523 Modular plugins

b427574

- ReducerManager, updated StandardStore to use it.

Store manager, wrapping epics to make them mutable per plugin

d002281

Making plugins lazy by default.

e0ca7bf

Passing lazy plugins into PluginsContainer component.

Intermediate commit, test changes to lazy load everything on demand.

8373c05

alexander-fedorenko self-assigned this Aug 2, 2022

alexander-fedorenko added 4 commits August 3, 2022 09:23

Review of the plugins that breaks the app when turned into lazy ones.

afe7925

Share plugin definition fix.

Mute / Unmute epics based on plugin availability on the page.

de4e1d4

Re-use plugins cache whenever hook is used by a new PluginsContainer instance. Cleanup, making user manager page works again.

Complete cleanup of pluginManager in favor of the hook-based implemen…

e69a4a2

…tation.

Cleanup

cce7b2b

tdipisa requested a review from allyoucanmap August 3, 2022 07:36

- Merging plugins and module plugins definitions.

684e399

- Moving Module plugin utils into separate file - Naming conventions: Lazy loaded plugins are "Module plugins"

tdipisa added enhancement Priority: High C169-Rennes-Métropole-2021-GeOrchestra3 labels Aug 3, 2022

alexander-fedorenko added 4 commits August 4, 2022 01:42

Minor change, renamed utility function.

aed713e

Minor change, renamed utility function.

2ff2341

tdipisa added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Aug 4, 2022

Loading should be presented, otherwise it won't work on loading state…

4dd993f

… change =

allyoucanmap reviewed Aug 4, 2022

View reviewed changes

alexander-fedorenko added 9 commits August 5, 2022 00:33

- Moved connect from PluginsContainer to withModulePlugins enhancer. …

12627de

…Included common values from state into definition so usage of PluginsContainer on page components become shorter and simpler.

Redundant compose removed

2a3cec9

- Changes in withExtensions to support modular extensions.

05990e0

- Fixes for test coverage.

Remaining static plugins turned into module plugins where possible

3f48867

Replaced require with imports

1856d94

UserSession should be static too

9af6ba7

Use ModulePluginsContainer as a container for plugins on MapStore pages.

21b0c24

alexander-fedorenko requested a review from allyoucanmap August 10, 2022 13:31

alexander-fedorenko added 2 commits August 11, 2022 03:32

Minor change to support syntax of extensions export.

d6f0a9e

- Documentation update;

1410d5d

- Restored augmentStore but marked it as deprecated

allyoucanmap approved these changes Aug 11, 2022

View reviewed changes

Normalize name prior to check of muteState

527a130

allyoucanmap merged commit 4005b4e into geosolutions-it:master Aug 11, 2022

This was referenced Aug 12, 2022

Wrong position of Home button in omnibar #8476

Closed

App Context Manager no longer works #8477

Closed

alexander-fedorenko mentioned this pull request Aug 18, 2022

#8503 Bugs related modular plugins implementation #8495

Merged

12 tasks

tdipisa mentioned this pull request Aug 30, 2022

Burger menu not visible in DEV for context #8523

Closed

1 task

alexander-fedorenko mentioned this pull request Sep 5, 2022

#8513 Properly pass epic name into isolation function, so that muteState has all the epics listed correctly. #8514

Merged

12 tasks

alexander-fedorenko added a commit to alexander-fedorenko/MapStore2 that referenced this pull request Sep 7, 2022

geosolutions-it#8423 Modular plugins (geosolutions-it#8457)

226b4af

(cherry picked from commit 4005b4e)

alexander-fedorenko mentioned this pull request Sep 7, 2022

[Backport 2022.02.xx] module plugins PRs #8558

Merged

ElenaGallo removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Sep 7, 2022

tdipisa added the current-release label Sep 28, 2022

offtherailz added this to the v2022.02.00 milestone Sep 28, 2022

offtherailz removed the current-release label Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8423 Modular plugins #8457

8423 Modular plugins #8457

alexander-fedorenko commented Aug 2, 2022 •

edited

Loading

allyoucanmap left a comment

allyoucanmap Aug 4, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 5, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

allyoucanmap Aug 4, 2022

alexander-fedorenko Aug 5, 2022

alexander-fedorenko commented Aug 10, 2022

allyoucanmap commented Aug 10, 2022

tdipisa commented Aug 10, 2022 •

edited

Loading

alexander-fedorenko commented Aug 11, 2022

tdipisa commented Aug 11, 2022 •

edited

Loading

tdipisa commented Aug 12, 2022

offtherailz commented Aug 16, 2022 •

edited

Loading

Bundle analyzer after recent updates

Static plugins included

Parsed

Gzipped

Static plugins excluded

Parsed

Gzipped

	const { plugins, ...rest } = this.props.pluginsDef;
	const { plugins, requires } = this.props.pluginsDef;

8423 Modular plugins #8457

8423 Modular plugins #8457

Conversation

alexander-fedorenko commented Aug 2, 2022 • edited Loading

Description

Issue

Breaking change

Other useful information

allyoucanmap 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fedorenko commented Aug 10, 2022

Bundle analyzer after recent updates

Static plugins included

Parsed

Gzipped

Static plugins excluded

Parsed

Gzipped

allyoucanmap commented Aug 10, 2022

tdipisa commented Aug 10, 2022 • edited Loading

alexander-fedorenko commented Aug 11, 2022

tdipisa commented Aug 11, 2022 • edited Loading

tdipisa commented Aug 12, 2022

offtherailz commented Aug 16, 2022 • edited Loading

Bundle analyzer after recent updates

Static plugins included

Parsed

Gzipped

Static plugins excluded

Parsed

Gzipped

alexander-fedorenko commented Aug 2, 2022 •

edited

Loading

tdipisa commented Aug 10, 2022 •

edited

Loading

tdipisa commented Aug 11, 2022 •

edited

Loading

offtherailz commented Aug 16, 2022 •

edited

Loading