Synchronous version of the new schema #2104

ychoquet · 2024-05-08T15:20:34Z

Description

Modification of the new config API to accept the new structure of the schema.

Fixes # 2103

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?

Using Chrome Devtools.

Deploy URL: https://ychoquet.github.io/GeoView/config-sandbox.html

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 8 of 13 files at r1, 3 of 4 files at r2.
Reviewable status: 10 of 14 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan and @ychoquet)

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

      enableRotation: true,
      rotation: 0,
      minZoom: 0,

These 3 attributes are still on viewSettings with default values I think... Especially maxExtent

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

          "description": "Maximum number of records to fetch (default: 0)."
        },
        "layerFilter": {

layerFilter should only be on vaector and esriDynamic... same for maxREcordCount. Seems riht in map-schema-type

packages/geoview-core/src/api/config/types/map-schema-types.ts line 126 at r1 (raw file):

  alias: string;
  type: TypeOutfieldsType;
  domaine: unknown[];

Typo domain

packages/geoview-core/src/api/config/types/map-schema-types.ts line 239 at r1 (raw file):

 * Initial settings for WMS image sources.
 */
export interface TypeSourceImageEsriInitialConfig extends TypeBaseSourceImageInitialConfig {

maxRecord will have no effect on WMS... layer filter will work (my mistake for my comment in file upper)

packages/geoview-core/src/api/config/types/map-schema-types.ts line 455 at r1 (raw file):

   * Default [-125, 30, -60, 89].
   */
  maxExtent?: Extent;

There 2 attributes should stay in viewSetting as it is max extent and min/max zoom to constrain the view

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

      : CV_DEFAULT_MAP_FEATURE_CONFIG.schemaVersionUsed!;

    const minZoom = this.map.viewSettings.minZoom!;

Same comment as above

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

   * @private
   */
  #validateMaxExtent(): void {

Same comment as above. If not maxExtent provided, we need the default one

packages/geoview-core/src/api/config/types/classes/sub-layer-config/raster-leaf/esri-dynamic-layer-entry-config.ts line 21 at r1 (raw file):

export class EsriDynamicLayerEntryConfig extends AbstractBaseLayerEntryConfig {
  /** Filter to apply on feature of this layer. */
  layerFilter?: string;

If I understand correctly, this is now part of the source info

jolevesq

Reviewed 1 of 13 files at r1, 1 of 4 files at r2.
Reviewable status: 12 of 14 files reviewed, 8 unresolved discussions (waiting on @Alex-NRCan and @ychoquet)

Alex-NRCan

Reviewed 3 of 13 files at r1, 1 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ychoquet)

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

          if (layerType === CV_CONST_LAYER_TYPES.ESRI_DYNAMIC && !isFeature) {
            const geoviewLayerConfig: TypeJsonObject = {
              geoviewLayerId: `${id}` as TypeJsonObject,

Here and in other places similar, to clarify, maybe best to create de json object without all the casting for each prop and do 1 single cast as TypeJsonObejct for the geoviewLayerConfig instead?
Example:

Code snippet:

const geoviewLayerConfig = {
  geoviewLayerId: `${id}`,
  geoviewLayerName: createLocalizedString(name as string),
  metadataAccessPath: createLocalizedString(url as string),
  geoviewLayerType: CV_CONST_LAYER_TYPES.ESRI_DYNAMIC,
} as unknown as TypeJsonObject;

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

    // Return the parsed response
    return this.getLayerConfigFromResponse(result, lang);

Here, I notice that it's not parsing the information related to the GeoChart anymore, that's intended?
GeoChart will not work on GeoCore layers if not.
It's not a change that happened in this current PR, but it's been removed at some point. It's still in the original configuration file.

jolevesq

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

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

Previously, Alex-NRCan (Alex) wrote…

Here, I notice that it's not parsing the information related to the GeoChart anymore, that's intended?
GeoChart will not work on GeoCore layers if not.
It's not a change that happened in this current PR, but it's been removed at some point. It's still in the original configuration file.

I dontknow if it should stay here in the readr or be responsability of layersAPI to get metadata for the plugins... This reader has been added in last PR to be able to load layer by url. It will need some more thingking. The original is still accessible and wont be removed until it is working

Alex-NRCan

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

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

Previously, Alex-NRCan (Alex) wrote…

Here and in other places similar, to clarify, maybe best to create de json object without all the casting for each prop and do 1 single cast as TypeJsonObejct for the geoviewLayerConfig instead?
Example:

Actually, even better if possible. Use the LayerEntryConfig typing instead of working with a TypeJsonObject which is basically an 'any' in disguise.

ychoquet

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

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

Previously, Alex-NRCan (Alex) wrote…

Actually, even better if possible. Use the LayerEntryConfig typing instead of working with a TypeJsonObject which is basically an 'any' in disguise.

Done. I used the Cast(value) which is essentially the same thing as unknown as TypeJsonObject.

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

Previously, jolevesq (Johann Levesque) wrote…

I dontknow if it should stay here in the readr or be responsability of layersAPI to get metadata for the plugins... This reader has been added in last PR to be able to load layer by url. It will need some more thingking. The original is still accessible and wont be removed until it is working

If it has to be restored, i'll do. However, I'm not sure it is part of the config.

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

Previously, jolevesq (Johann Levesque) wrote…

These 3 attributes are still on viewSettings with default values I think... Especially maxExtent

Done.

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

Previously, jolevesq (Johann Levesque) wrote…

layerFilter should only be on vaector and esriDynamic... same for maxREcordCount. Seems riht in map-schema-type

Done. This is the case.

packages/geoview-core/src/api/config/types/map-schema-types.ts line 126 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

Typo domain

Done.

packages/geoview-core/src/api/config/types/map-schema-types.ts line 239 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

maxRecord will have no effect on WMS... layer filter will work (my mistake for my comment in file upper)

Done. The error is in the header. This is the ESRI source definition.

packages/geoview-core/src/api/config/types/map-schema-types.ts line 455 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

There 2 attributes should stay in viewSetting as it is max extent and min/max zoom to constrain the view

Done.

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

Previously, jolevesq (Johann Levesque) wrote…

Same comment as above

Done.

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

Previously, jolevesq (Johann Levesque) wrote…

Same comment as above. If not maxExtent provided, we need the default one

Done.

packages/geoview-core/src/api/config/types/classes/sub-layer-config/raster-leaf/esri-dynamic-layer-entry-config.ts line 21 at r1 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

If I understand correctly, this is now part of the source info

Yes.

…elds

jolevesq

Reviewed 7 of 8 files at r3, all commit messages.
Reviewable status: 14 of 15 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)

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

Previously, ychoquet (Yves Choquette) wrote…

If it has to be restored, i'll do. However, I'm not sure it is part of the config.

It may be part of this uuid reader file like before. We will do in another PR

ychoquet

Reviewable status: 5 of 15 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan and @jolevesq)

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

Previously, jolevesq (Johann Levesque) wrote…

It may be part of this uuid reader file like before. We will do in another PR

Ok!

jolevesq

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

ychoquet force-pushed the Implementation_of_the_new_schema branch from 8d06a8a to cf82921 Compare May 8, 2024 16:31

ychoquet marked this pull request as ready for review May 8, 2024 16:38

jolevesq requested review from jolevesq and Alex-NRCan May 8, 2024 17:01

jolevesq requested changes May 8, 2024

View reviewed changes

jolevesq reviewed May 8, 2024

View reviewed changes

Alex-NRCan requested changes May 8, 2024

View reviewed changes

jolevesq reviewed May 8, 2024

View reviewed changes

Alex-NRCan reviewed May 8, 2024

View reviewed changes

ychoquet commented May 8, 2024

View reviewed changes

Integration of the source property in the schema and renaming some fi…

7e443c2

…elds

ychoquet force-pushed the Implementation_of_the_new_schema branch from cf82921 to 7e443c2 Compare May 8, 2024 23:03

jolevesq reviewed May 8, 2024

View reviewed changes

Revert property names to what it was before

675600d

ychoquet commented May 9, 2024

View reviewed changes

jolevesq approved these changes May 9, 2024

View reviewed changes

jolevesq merged commit 05af875 into Canadian-Geospatial-Platform:develop May 9, 2024
5 of 6 checks passed

ychoquet self-assigned this Jul 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronous version of the new schema #2104

Synchronous version of the new schema #2104

ychoquet commented May 8, 2024 •

edited by jolevesq

Loading

jolevesq left a comment

jolevesq left a comment

Alex-NRCan left a comment

jolevesq left a comment

Alex-NRCan left a comment

ychoquet left a comment

jolevesq left a comment

ychoquet left a comment

jolevesq left a comment

Synchronous version of the new schema #2104

Synchronous version of the new schema #2104

Conversation

ychoquet commented May 8, 2024 • edited by jolevesq Loading

Description

Type of change

How Has This Been Tested?

Checklist:

jolevesq left a comment

Choose a reason for hiding this comment

jolevesq left a comment

Choose a reason for hiding this comment

Alex-NRCan left a comment

Choose a reason for hiding this comment

jolevesq left a comment

Choose a reason for hiding this comment

Alex-NRCan left a comment

Choose a reason for hiding this comment

ychoquet left a comment

Choose a reason for hiding this comment

jolevesq left a comment

Choose a reason for hiding this comment

ychoquet left a comment

Choose a reason for hiding this comment

jolevesq left a comment

Choose a reason for hiding this comment

ychoquet commented May 8, 2024 •

edited by jolevesq

Loading