Fixes #3454 plugins developers documentation - round 2

[mbarto]

Description

Second round of developers documentation for plugins-

Issues

• Fix Dev Guide - Writing plugins #3454

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)

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• [ x Other... Please describe: documentation

What is the current behavior? (You can also link to an open issue here)
Incomplete plugins documentation

What is the new behavior?
More complete plugins documentation

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

• Yes
• No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[offtherailz]

I should suggest to put the selectors in the global file anyway, with common best practices of code reuse.
In my opinion they should be state access interface, that increases testability, stability, maintainability and so on.

We of course have to distinguish between selectors that access to common state parts and selectors that map state to specific plugin properties.
The latters should be in the plugin.
What do you think?

[mbarto]

I do not agree, putting anything in separate files decreases readability of the code, so we should put them in a separate file only when they are really shared.
Code re-use literally means "more than one use".

[offtherailz]

I understand your point. please take into account also the following considerations:

1. When you develop a plugin, to understand if your selector already exists you have to look at all the plugins you think can have a similar behavior instead of look at the selector's file of the state splice you want to read. This of course encourages duplication. Instead having a file of already done function encourages the reuse of them.

2. Having the logic to access to the global state parts in the plugins decrease readability:

See this (not real) example of a tool that for epsg4326, if feature info is disabled, do geocoding on click :

This is what you propose

connect( () => createSelector(
  state => state.map && state.map.projection && state.map.prolection === "EPSG:4326",
  state => state.controls && state.controls.featureinfo && !state.controls.featureinfo.enabled ,
  state => state.geocoder && state.geocoder.services && state.geocoder.services.length > 0,
  (isCompatibleProjection, isFeatureInfoDisabled) => ({
  myProp: isCompatibleProjection && isFeatureInfoDisabled
}))

I think this version is more readable, hiding the state access complexity

connect( () => createSelector(
  isGeocodingEnabled,
  enabled => ({enabled}) // map to your props
 )

this in selectors/geocoder.js:

/**
 * Check if the geocoder tool should be enabled (it is enabled only for  EPSG:4326, when at least 
 * one service is available and feature info has not been activated
 */
const isGeocodingEnabled = createSelector(
  state => state.map && state.map.projection && state.map.projection === "EPSG:4326",
  state => state.controls && state.controls.featureinfo && !state.controls.featureinfo.enabled,
  state => state.geocoder && state.geocoder.services && state.geocoder.services.length > 0,
  (isCompatibleProjection, isFeatureInfoDisabled, hasServices) => isCompatibleProjection && FeatureInfoDisabled && hasServices
);

This:

• easy to document and test
• ready for reuse
• easy to maintain
• easy to refactor
• With all the selectors in the same file you can quickly identify duplications and parts that can be reused inside the same selector. The above selector can be easly converted into

const isEnabled = createSelector(
  state => projectionSelector(state) === "EPSG:4326", // from selectors/map.js
  state => !isControlEnabled("featureinfo")(state), // from controls
  state => servicesSelector(state).length > 0 // inside this file,
  (isCompatibleProjection, isFeatureInfoDisabled, hasServices) => isCompatibleProjection && FeatureInfoDisabled && hasServices
);

[offtherailz]

Does it means I convinced you or you're going to shoot me a rocket 😆 ?