refactor(layers) - Various enhancements around the layers including architecture rearrangement in preparation for bigger changes, new rules #2149

Alex-NRCan · 2024-05-16T20:23:21Z

Description

Spring-board PR in preparation for bigger changes. This PR stabilizes what we have without, hopefully, breaking anything.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Hosted, May 16th, 16h30: https://alex-nrcan.github.io/geoview/

Checklist:

I have build (rush build) and deploy (rush host) my PR
I have connected the issues(s) to this PR
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have created new issue(s) related to the outcome of this PR is needed
~~I have made corresponding changes to the documentation~~
~~I have added tests that prove my fix is effective or that my feature works~~
~~New and existing unit tests pass locally with my changes~~

This change is

jolevesq

Reviewed 30 of 51 files at r1, all commit messages.
Reviewable status: 30 of 51 files reviewed, 18 unresolved discussions (waiting on @Alex-NRCan)

packages/geoview-core/public/templates/ui-components.html line 178 at r1 (raw file):

                const field = cgpv.api.maps.UI1.layer.getGeoviewLayer('historical-flood/0').getTemporalDimension('historical-flood/0').field;
                cgpv.api.maps.UI1.layer
                  .getGeoviewLayer('historical-flood/0')

I like the naming prefix with get for function name

packages/geoview-core/src/app.tsx line 131 at r1 (raw file):

  // create a new config for this map element
  const config = new Config();
  const configObj = await config.initializeMapConfig(configuration.mapId, configuration!.map!.listOfGeoviewLayerConfig!);

I see no warning in the console for this unneeded await, only ... under it. You add a rule or it is just your eye?

packages/geoview-core/src/core/utils/config/reader/uuid-config-reader.ts line 102 at r1 (raw file):

                  dataAccessPath: createLocalizedString(url as string),
                },
              } as unknown as EsriDynamicLayerEntryConfig);

Why do you need the unknown only for this type of layer?

packages/geoview-core/src/core/utils/config/validation-classes/abstract-base-layer-entry-config.ts line 118 at r1 (raw file):

   * @param {LayerGroup} olLayerValue The new olLayerd value.
   */
  set olLayer(olLayerValue: BaseLayer | LayerGroup | null) {

Good thing to move this ol outside of the config validation class

packages/geoview-core/src/core/utils/config/validation-classes/config-base-class.ts line 79 at r1 (raw file):

  static #layerStatusWeight = {
    newInstance: 10,

We need to put a TODO to review the status. I think we should have: newInstance, processsing, loading, - loaded : error

packages/geoview-core/src/core/utils/config/validation-classes/config-base-class.ts line 106 at r1 (raw file):

  get layerId(): string {
    // eslint-disable-next-line no-underscore-dangle
    return this._layerId;

This is internal to layer, what is it use for? What the difference with layerPath

packages/geoview-core/src/core/utils/config/validation-classes/config-base-class.ts line 126 at r1 (raw file):

  get layerPath(): string {
    // TODO: Refactor - It would be better to not have a 'getter' that 'sets' a value at the same time.
    // TO.DOCONT: Unfortunately, when commenting this out (to rely on the one in layerId) things almost work, except for the Groups inside Groups which don't.

Should be TD.CONT?

packages/geoview-core/src/core/utils/config/validation-classes/config-base-class.ts line 207 at r1 (raw file):

   */
  static #evaluateLayerPath(layerConfig: ConfigBaseClass, layerPath?: string): string {
    let pathEnding = layerPath;

I think we are not using pathEnding anymore... @ychoquet can you confirm

packages/geoview-core/src/core/utils/config/validation-classes/config-base-class.ts line 264 at r1 (raw file):

    // TODO: Check - Move this registerToLayerSets closer to the others, when I comment the line the Groups start breaking
    // TO.DOCONT: See if we can fix this elsewhere and remove this.

TD.CONT

packages/geoview-core/src/geo/layer/layer.ts line 68 at r1 (raw file):

  /** Layers with valid configuration for this map. */
  // TODO: Refactor - Turn this #private if we don't want developers to be able to push (or delete) in this array in an
  // TO.DOCONT: attempt to register layers without going through the proper process. Any other modifiers aren't enough, due to calls from pure JS.

TD.CONT

packages/geoview-core/src/geo/layer/layer.ts line 204 at r1 (raw file):

    }

    // Wait for all promises (GeoCore ones) to process

We can add a note, there should be no Geocore layer in layers in the geo folder. Geocore are translated to their ol layer type in config when we will use new config for layers

packages/geoview-core/src/geo/layer/layer.ts line 443 at r1 (raw file):

        // TODO: Check - If we're doing the assignation here, is it still necessary to do it in setLayerAndLoadEndListeners!?
        // eslint-disable-next-line no-param-reassign
        layerConfig.geoviewLayerInstance = geoviewLayer;

For the future, we should not attach a geoviewLAyer to a layer config... the geoviewLAyer is the instance and the config is use to create it. Then it can be attach for reference/

packages/geoview-core/src/geo/layer/layer.ts line 575 at r1 (raw file):

   */
  unregisterLayer(layerConfig: TypeLayerEntryConfig): void {
    // // Register a handler on the layer config to track the status changed

Do we need this dead code?

packages/geoview-core/src/geo/layer/layer.ts line 942 at r1 (raw file):

    if (layerEntryIsGroupLayer(this.registeredLayers[layerPath] as TypeLayerEntryConfig)) {
      Object.keys(this.registeredLayers).forEach((registeredLayerPath) => {
        const theLayer = this.getGeoviewLayer(registeredLayerPath)!;

Much easier to follow this way with the getGeoviewLayer function

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 68 at r1 (raw file):

 */
export abstract class AbstractGeoViewLayer {
  // TODO: Refactor - The layer object should probably not be aware of a "map id". Check if necessary and redesign.

The thing that can be good about it is if a layer is use in an event processor, it will carry the mapId with him.... but there is other way to get it so this can be remove as well

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 105 at r1 (raw file):

   * The OpenLayer root layer representing this GeoView Layer.
   */
  olRootLayer?: BaseLayer | LayerGroup;

I think it is a good idea to have base or group.... no null anymore

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 313 at r1 (raw file):

  /**
   * Registers a legend queried event handler.

it is not legend query but layer creation event

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 322 at r1 (raw file):

  /**
   * Unregisters a legend queried event handler.

it is not legend query but layer creation event

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 906 at r1 (raw file):

   */
  protected static createLayerGroup(layerConfig: TypeLayerEntryConfig, initialSettings: TypeLayerInitialSettings): LayerGroup {
    // TODO: Move this function and especially the assignation to  layerConfig.olLayer below, elsewhere than in this class

And ol layer should be on the geoviewLayer instance not layerConfig.

packages/geoview-core/src/geo/layer/geoview-layers/abstract-geoview-layers.ts line 1479 at r1 (raw file):

    const { olLayer, loadEndListenerType } = layerAndListenerType;
    // eslint-disable-next-line no-param-reassign, no-underscore-dangle
    layerConfig._olLayer = olLayer;

Again here, for next refactor, we should not have stuff attach to the layerConfig. The geoviewLAyer instance should do this.... At the end it may just be renaming stuff

packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 37 at r1 (raw file):

 * @returns {Promise<void>} A promise that the execution is completed.
 */
export async function commonfetchServiceMetadata(layer: EsriDynamic | EsriFeature): Promise<void> {

Good renaming

jolevesq

Reviewed 21 of 51 files at r1.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Alex-NRCan)

packages/geoview-core/src/geo/layer/layer-sets/abstract-layer-set.ts line 82 at r1 (raw file):

          this.resultSet[layerPath].layerName = getLocalizedValue(
            {
              en: `Anonymous Layer ${this.anonymousSequenceNumber}`,

Good to remove as there is no value for the number

packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 55 at r1 (raw file):

   * @param {TypeLayerEntryConfig} layerConfig - The layer config
   */
  protected override onRegisterLayer(geoviewLayer: AbstractGeoViewLayer, layerConfig: TypeLayerEntryConfig): void {

Nice removing of ... kind of duplicate ... parameters as geoviewLayer and layerConfig should be kind of one at the end

packages/geoview-core/src/geo/map/map-schema-types.ts line 295 at r1 (raw file):

export type TypeLayerEntryConfig =
  | AbstractBaseLayerEntryConfig
  | VectorLayerEntryConfig

Good stuff, cleaning this will make our life easier when it will be time to disconnect this map-schema-type

packages/geoview-core/src/geo/map/map-schema-types.ts line 314 at r1 (raw file):

 */
// TODO: Suggestion - Get rid of this type. Simply use TypeGeoviewLayerConfig[]. It'd simplify types management accross the source code.
export type TypeListOfGeoviewLayerConfig = TypeGeoviewLayerConfig[];

Good!

packages/geoview-time-slider/src/index.tsx line 141 at r1 (raw file):

        const layerPath = obj.layerPaths[0];
        const timeDimension = MapEventProcessor.getMapViewerLayerAPI(this.pluginProps.mapId)
          .getGeoviewLayer(layerPath)

Here we should have a distintion between geoviewLAyer (root) and leaf of a geviewLayer and have different paramter like getGeoviewLayer(layerId).getTemporalDimension(LayerPAth)

Just something to show the difference between root and leaf... or I may miss something in the call

Alex-NRCan

Reviewed 51 of 51 files at r1, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jolevesq)

packages/geoview-core/public/templates/ui-components.html line 178 at r1 (raw file):