Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Consolidate arcgis rest js #1497

Merged
merged 15 commits into from
Sep 10, 2024
Merged

Conversation

previnWong
Copy link
Collaborator

This PR consolidates all the interfaces, classes, and functions exports from ArcGIS Rest JS in various files into a single file arcgisRestJS.ts in packages/common.

All references in various files have been updated from direct arcgis-rest-js import/exports into point to this file.

All arcgis rest js exports from common/interfaces has also been moved to arcgisRestJS

For function that have custom exports in restHelpers that has the same name as an export in arcgis-rest-js, the export from arcgis-rest-js has "rest" added to the export name. Example image below
image

previnWong and others added 11 commits August 29, 2024 16:54
move all arcgis-rest-js references to common.
consolidating arcgis-rest-js imports into a single file.  Changed reference from files to the file.
updated test files
lint clean up
fix test script errors
remove interface prefix on test
@previnWong previnWong self-assigned this Sep 10, 2024
Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2cde893) to head (b8a55a3).
Report is 16 commits behind head on develop.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop     #1497   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          150       151    +1     
  Lines         8082      8109   +27     
  Branches      1797      1854   +57     
=========================================
+ Hits          8082      8109   +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MikeTschudi MikeTschudi marked this pull request as draft September 10, 2024 18:24
@MikeTschudi MikeTschudi marked this pull request as ready for review September 10, 2024 19:08
jmhauck
jmhauck previously approved these changes Sep 10, 2024
@MikeTschudi
Copy link
Member

@previnWong & @jmhauck, I added three commits

Copy link
Member

@MikeTschudi MikeTschudi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Previn!

A next step is to remove arcgis-rest-* from the rest of the test code so that when we change from 3.x to 4.x, the tests should continue to work.

The automatic formatting is a pain the first time, but will eliminate false change flags in the future.

@previnWong
Copy link
Collaborator Author

Mike, what other tests needs to be updated?

@MikeTschudi
Copy link
Member

Here's what I found searching for "@esri/arcgis-rest"

File solution.js\packages\common\test\create-hub-request-options.test.ts
  18 32:import * as portalModule from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\deleteHelpers\removeItems.test.ts
  23 26:import * as portal from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\deleteSolution.test.ts
  31 26:import * as portal from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\featureServiceHelpers.test.ts
  85 38:import * as rest_feature_layer from "@esri/arcgis-rest-feature-layer";
File solution.js\packages\common\test\generalHelpers.test.ts
  25 32:import * as serviceAdmin from "@esri/arcgis-rest-service-admin";
File solution.js\packages\common\test\resourceHelpers.test.ts
  23 26:import * as portal from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\resources\copyAssociatedFiles.test.ts
  38 26:import * as portal from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\restHelpers.test.ts
  22 25:import * as admin from "@esri/arcgis-rest-service-admin";
  37 26:import * as portal from "@esri/arcgis-rest-portal";
  38 27:import * as request from "@esri/arcgis-rest-request";
  46 32:import { IPagingParams } from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\restHelpersGet.test.ts
  24 26:import * as portal from "@esri/arcgis-rest-portal";
  25 27:import * as request from "@esri/arcgis-rest-request";
File solution.js\packages\common\test\sharing\share-item-to-groups.test.ts
  18 26:import * as portal from "@esri/arcgis-rest-portal";
File solution.js\packages\common\test\workflowHelpers.test.ts
  22 27:import * as request from "@esri/arcgis-rest-request";
File solution.js\packages\form\test\helpers\create-item-from-hub-template.test.ts
  18 30:import * as restPortal from "@esri/arcgis-rest-portal";
File solution.js\packages\hub-types\test\helpers\replace-item-ids.test.ts
  19 24:import { IItem } from "@esri/arcgis-rest-portal";
File solution.js\packages\hub-types\test\helpers\_post-process-page.test.ts
  20 38:import { IUpdateItemResponse } from "@esri/arcgis-rest-portal";
File solution.js\packages\hub-types\test\helpers\_post-process-site.test.ts
  20 38:import { IUpdateItemResponse } from "@esri/arcgis-rest-portal";
File solution.js\packages\storymap\test\helpers\get-storymap-dependencies.test.ts
  19 24:import { IItem } from "@esri/arcgis-rest-portal";
File solution.js\packages\web-experience\test\helpers\get-web-experience-dependecies.test.ts
  19 24:import { IItem } from "@esri/arcgis-rest-portal";

@previnWong
Copy link
Collaborator Author

geez, I thought I did a wildcard search and caught them all. Sheesh. I'll start on those ones. Good to merge this PR first though?

@MikeTschudi
Copy link
Member

If your review of my three commits is OK, then, yes, please, let's merge this PR. Makes the future work easier to review.

@previnWong previnWong merged commit f0e6257 into develop Sep 10, 2024
9 checks passed
@previnWong previnWong deleted the pw-consolidate-arcgis-rest-js branch September 10, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants