Reviewed 3 of 22 files at r1, all commit messages.
Reviewable status: 3 of 22 files reviewed, 3 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/config-api.ts line 229 at r1 (raw file):

            urlParams.keys.toString().split(',')
          );
          const listOfGeoviewLayerConfig = await promise;

Add blank lines between section of code and some comments... what this section is doing?

packages/geoview-core/src/api/config/config-api.ts line 472 at r1 (raw file):

        const layerConfig = { geoviewLayerId: serviceAccessString, geoviewLayerType: layerType };
        geoviewLayerConfig = (await ConfigApi.convertGeocoreToGeoview(language, Cast<TypeJsonObject>(layerConfig))) as TypeJsonObject;
        if (!geoviewLayerConfig) return undefined;

Add blank lines and comments

packages/geoview-core/src/api/config/config-api.ts line 503 at r1 (raw file):

   * @static
   */
  // GV: GeoCore layers are processed here, well before the schema validation. The aim is to get rid of these layers in

There is process og geocore here... it is just a layerTree extraction. We can even remove geocore from this function as we do not want layertree for geoCore

Reviewable status: 3 of 22 files reviewed, 4 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/config-api.ts line 511 at r1 (raw file):

    language: TypeDisplayLanguage = 'en'
  ): Promise<EntryConfigBaseClass[]> {
    const geoviewLayerConfig = await ConfigApi.createLayerConfig(serviceAccessString, layerType, listOfLayerId, language);

We do not want to create a layer config here.... really just fetch metadata and create layer tree

Reviewed 7 of 22 files at r1.
Reviewable status: 10 of 22 files reviewed, 11 unresolved discussions (waiting on @ychoquet)

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

              (item): TypeJsonObject => {
                return Cast<TypeJsonObject>({
                  entryType: CV_CONST_SUB_LAYER_TYPES.RASTER_IMAGE,

There is no entry type anymore or it has been moved somewhere else?

packages/geoview-core/src/api/config/types/config-validation-schema.json line 438 at r1 (raw file):

        }
      },
      "required": ["geoviewLayerId", "geoviewLayerName", "geoviewLayerType", "listOfLayerEntryConfig"]

and name not needed anymore?

packages/geoview-core/src/api/config/types/classes/config-exceptions.ts line 42 at r1 (raw file):

  messageList: Record<string, string> = {
    LayerTypeMandatory: 'Property geoviewLayerType is mandatory for GeoView layer <=> of type <=>.',
    LayerIdMandatory: 'Property geoviewLayerId is mandatory for GeoView layer of type <=>.',

We do not enforce anymore?

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 14 at r1 (raw file):

import { getXMLHttpRequest } from '@/core/utils/utilities';
import { logger } from '@/core/utils/logger';
import { TypeJsonArray } from '@/app';

I think app is not the proper path

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 24 at r1 (raw file):

   * @param {TypeDisplayLanguage} language The initial language to use when interacting with the map feature configuration.
   */
  constructor(geoviewLayerConfig: TypeJsonObject, language: TypeDisplayLanguage) {

Why we do not pass only url and list of ids as it is the only needed thing to create metadata. The rest of config will be use later in a applyUserConfig function.

Less management in the constructor

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 85 at r1 (raw file):

          });
        };
        processListOfLayerEntryConfig(this.listOfLayerEntryConfig);

Blank line before, add some comments

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 87 at r1 (raw file):

        processListOfLayerEntryConfig(this.listOfLayerEntryConfig);
        await this.#fetchListOfLayerMetadata(this.metadataLayerTree);
        this.metadataLayerTree = this.createLayerTree();

We do not need metadata tree here, it is only needed in the configAPI getMetadataTree function? You do not need the whole tree if you have lets say 2 ids

If is it another function that does not relate to the function getMEtadataLAyerTrtee we need better naming

Reviewable status: 10 of 22 files reviewed, 12 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 23 at r1 (raw file):

  /** Cloned copy of the configuration as provided by the user when the constructor was called. */
  #geoviewLayerConfig: TypeJsonObject;

I do not think we need this... we should create config from metadata and have a function who take the piece of cofig to apply on top.... We can reuse the same object to get different config

Reviewable status: 10 of 22 files reviewed, 13 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 76 at r1 (raw file):

    // Keep a copy of the configuration. It will be used later in the execution flow to overwrite values obtained from the metadata.
    this.#geoviewLayerConfig = cloneDeep(geoviewLayerConfig);
    this.#validateGeoviewConfig();

VAlidate config is made when we validate map for layer config schema? We should then validate only in the applyUIserConfig function

Reviewed 6 of 22 files at r1.
Reviewable status: 16 of 22 files reviewed, 14 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 89 at r1 (raw file):

    let jsonListOfLayerEntryConfig: TypeJsonArray;
    switch ((geoviewLayerConfig?.listOfLayerEntryConfig as TypeJsonArray)?.length) {
      case undefined:

iCan t be undefined?

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/config-api.ts line 465 at r1 (raw file):

  ): Promise<AbstractGeoviewLayerConfig | undefined> {
    let geoviewLayerConfig: TypeJsonObject | undefined;
    const listOfLayerEntryConfig = listOfLayerId.map((layerId) => {

Minor
This listOfLayerEntryConfig seem only used in the 'else' condition. Move the code below to help understand what it's used for when reading the code?

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 208 at r1 (raw file):

    const result = await Promise.allSettled(listOfLayerMetadata);
    logger.logInfo('fetchListOfLayerMetadata done', result);

Isn't it logDebug here? Because logInfo is for everyone and the message isn't really explanatory for common folks. Maybe adjust the message if meant for everyone?

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 61 at r1 (raw file):

   * Initial settings to apply to the GeoView layer at creation time.
   */
  initialSettings!: TypeLayerInitialSettings;

A bit hard to validate via reviewable, but I probably wouldn't add a ! to bypass the EsLint error in this case.
Can it be initialized to CV\_DEFAULT\_LAYER\_INITIAL\_SETTINGS straight away or else be typed as TypeLayerInitialSettings | undefined? Relying on the override of the applyDefaultValuesToUndefinedFields function seems a bit "far".

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 144 at r1 (raw file):

   *
   */
  applyDefaultValueToUndefinedFields(): void {

Question
Is this function used? I couldn't find where it was called.
Also, when reading the code, I got confused with the other functions named exactly the same in EntryConfigBaseClass (and its children classes) which functions receive an 'initialSettings' parameter in their signature while this one, in AbstractGeoviewLayerConfig, doesn't. Normal?

packages/geoview-core/src/api/config/types/classes/sub-layer-config/abstract-base-layer-entry-config.ts line 20 at r1 (raw file):

  /** The geometry type of the leaf node. */
  geometryType!: TypeStyleGeometry;

I wouldn't add a ! suffix on this attribute. Especially since I can't find where this attribute is defined/used in this class. Seems misleading.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 39 at r1 (raw file):

  /** Attributions obtained from the configuration or the metadata. */
  attributions!: string[];

I wouldn't add a ! suffix on this attribute. Especially since I can't find where this attribute is defined/used in this class. Seems misleading.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 45 at r1 (raw file):

  /** The min scale that can be reach by the layer. */
  minScale!: number;

I wouldn't add a ! suffix on this attribute. Especially since I can't find where this attribute is defined/used in this class. Seems misleading.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 48 at r1 (raw file):

  /** The max scale that can be reach by the layer. */
  maxScale!: number;

I wouldn't add a ! suffix on this attribute. Especially since I can't find where this attribute is defined/used in this class. Seems misleading.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 57 at r1 (raw file):

   * configuration tree.
   */
  initialSettings!: TypeLayerInitialSettings;

Same as other comment on initialSettings attribute

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @jolevesq)

packages/geoview-core/src/api/config/config-api.ts line 229 at r1 (raw file):

Done.
I added the following comment:
          // The listOfGeoviewLayerConfig returned by the previous call appended 'rcs.' at the beginning and
          // '.en' or '.fr' at the end of the UUIDs. We want to restore the ids as they were before.

packages/geoview-core/src/api/config/config-api.ts line 465 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/config-api.ts line 472 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/config-api.ts line 503 at r1 (raw file):

I don't understand why you want to remove the layer tree generation for GeoCore. If you go to the config-sandbox and request the layer tree for the GeoCore sample, it works and the user could request that only layer 1 - "Strontium-90 in Milk" be added to the map. If you absolutely want to remove it, I'll do it, but why remove something that's working properly?

packages/geoview-core/src/api/config/config-api.ts line 511 at r1 (raw file):

This is what I do. The GeoView layer instantiation just assigns the bare minimum of properties (metadataAccesPath, layerIds) and performs a few other quick tasks to select the right type of object that knows exactly how to read the metadata and create the layer tree. At the end, all we have to do is return the calculated layer tree. This approach is fully functional and encapsulates the processing linked to a layer type in an object that contains only code linked to its class. That's the beauty of object-oriented languages.

This avoids the need for a procedure containing a switch for each layer type, which connects to another sub-procedure that fetches the metadata and then connects to another sub-procedure that calculates the layer tree, thus scattering code logic everywhere.

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

The entryType propriety belongs to the internal schema. Its value is derived programmatically from the GeoView layer type. Users must not provide it. We already had a problem in the past where a user provided a wrong value for this property thinking that he can force an ESRI

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @jolevesq)

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

We already had a problem in the past where a user provided a wrong value for this property thinking that he can force an ESRI to behave like a WMS layer.

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @ychoquet)

packages/geoview-core/src/api/config/config-api.ts line 503 at r1 (raw file):

There is no layer tree for geocore. geocore is a whole config and user cannot select or unselect withing what is set.

packages/geoview-core/src/api/config/config-api.ts line 511 at r1 (raw file):

Good, as long as there is no unneeded call to fetch stuff specific to layer I am good.

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)

packages/geoview-core/src/api/config/config-api.ts line 503 at r1 (raw file):

I understand that GeoCore layers are considered as a whole, but when the GeoCore layer is converted to a GeoView layer, it is now possible to get a layer tree and use it in the add new layer panel to select and load only one sublayer from the tree.

packages/geoview-core/src/api/config/types/config-validation-schema.json line 438 at r1 (raw file):

It is still possible to specify it, but it is not mandatory. However, "metadataAccessPath" is missing in the required fields.

packages/geoview-core/src/api/config/types/classes/config-exceptions.ts line 42 at r1 (raw file):

This error message is never used because if the layerId is undefined, we assign a value to it using the generateId utility. So, the layerId property is never undefined in our code.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 14 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 24 at r1 (raw file):

Mainly for code simplicity and reusability, when we create a map as provided in the HTML template, the GeoView layers are provided as an array of GeoView Layer configuration JSON object. It is also the case for config loaded in a programmatic way. This is the most common way of providing the GeoView layer list in our code. This implementation simplify the code in the loop that process the list of layers when comes the time to process a map configuration.

Also, Keep in mind that abstract-geoview-esri-layer-config  was created to encapsulate the logic tha is common to the feature and dynamic sub-classes.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 85 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 87 at r1 (raw file):

As I mentioned above, creation of the layer tree is done here because each layer knows how to fetch its service metadata and compute the layer tree. It takes only a tiny fraction of a second to compute it from the service metadata. The result is kept internally and can be obtained using its getter when needed. This allows us to encapsulate all process related to a layer type in its classe hierarchy.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-esri-layer-config.ts line 208 at r1 (raw file):

You right, I removed it . I don't need it anymore.

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 23 at r1 (raw file):

I think we still need it because the user can provide its config in the map template or in a config file processed programmaticaly. This doesn't eliminate the fact that we want to apply a user config using another method. The two method will be available and well documented when we come to it.

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @Alex-NRCan and @jolevesq)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 61 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 76 at r1 (raw file):

We must validate the properties used in the instanciation to make sure we can proceed with the metadata fetch. The validation cost is only a tiny fraction of a second and it simplify the error managment that must be done later.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 89 at r1 (raw file):

geoviewLayerConfig is a flat json object comming from the users and the constructor is used to instanciate a GeoView layer in many situations. There is no guarantee that the user provided a valid structure since it has never been validated before.. This is why we use ?. in the switch variable so if geoviewLayerConfig?.listOfLayerEntryConfig?.length cannot be evaluated, we get undefined.

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 144 at r1 (raw file):

It will be in the next phase. Johann specification says that the default is applied after the metadata has been read and processed. That's the point we are now.

It's also worth noting that sublayers inherit the initialSettings from the parent container as default values. The abstractGeoviewLayerConfig is at the top of this inheritance and these initialSettings are propagated as new default values to the sublayers, which explains the presence of the parameter only for descendants of the EntryConfigBaseClass.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/abstract-base-layer-entry-config.ts line 20 at r1 (raw file):

Done. The geometryType may be undefined for some GeoView layers.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 39 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 45 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 48 at r1 (raw file):

Done.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 57 at r1 (raw file):

Done.

Reviewed 2 of 22 files at r1, 9 of 10 files at r2, all commit messages.
Reviewable status: 21 of 22 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan)

Reviewed 9 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ychoquet)

packages/geoview-core/src/api/config/types/classes/geoview-config/abstract-geoview-layer-config.ts line 144 at r1 (raw file):

I'd prefer to have a distinct function name to differentiate them from the overridable functions of the same name (if they're meant to be called differently), to remove possible confusion, but I can mark this as resolved.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 113 at r2 (raw file):

   */
  applyDefaultValueToUndefinedFields(initialSettings: TypeLayerInitialSettings): void {
    this.initialSettings = defaultsDeep(this.initialSettings, initialSettings);

This comment relates to my other couple comments about the '!' suffix.

I see you've added this section, which helps understand when those attributes are meant to be set/defaulted - which is great. Unfortunately, you're saying the code calling this function is only coming "in the next phase". Therefore, right now, it's hard to validate if your reasoning for bypassing the EsLint is "worth it". Until I can see when and how this method is called it's a grey area. I can resolve this for now.

Reviewable status: 21 of 22 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)

packages/geoview-core/src/api/config/types/classes/sub-layer-config/entry-config-base-class.ts line 113 at r2 (raw file):

Maybe add a TODO to validate when next phase is done

Reviewed 1 of 10 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)