Add support for Access Token when creating an ArcGISMapServerImageryProvider

[Tamrat-B]

Added support for ArcGIS Access Token when creating an ArcGisMapServerImageryProvider. Updated samples as well.

[cesium-concierge]

Thanks for the pull request @Tamrat-B!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.

[ggetz]

Thanks @Tamrat-B! Great to get the most up-to-date servers in!

A few items are needed before we can merge this PR:

• We'll want to update the release guide with instructions on how to rotate the token.
• There are a few failing unit tests. We'll need those to be passing.
• CHANGES.md will need to be updated with with a summary of the changes.

[gowin20]

@ggetz Hi Gabby! I'm George, the Esri developer who built our the ArcGIS x CesiumJS guide. We'd like to merge this PR ASAP so that it can fit into next week's Cesium build. Our CesiumJS guide is also slated for release next week (March 1st), but this PR needs to be merged before our guide can release. If we get this PR merged in time, we will be able to showcase a detailed Cesium integration at the Esri Developer Summit, which will benefit all parties.

Can you help accelerate this review process? Tamrat is currently traveling and is unable to make contributions for the next few days. I will address the language concerns in your review, but I'm new to the repository so it will take me some time to fix the nontrivial issues.

Any aspect of the review that you could assist with would be much appreciated. We could also delay some of the less important suggestions to a future release in order to merge a "version one" of this enhancement quickly. Thanks for your time.

[ggetz]

@gowin20 Hi George, nice to meet you! We understand your urgency around having this PR merged. Is it still possible for you to address my comments above? If not, let me know where you'd want some additional help. For example, I'm happy to updates the release guide or help with unit tests.

[ggetz]

Hi @gowin20 and @Tamrat-B, just wanted to bump my comment above. To get this change into the upcoming release on March 1, this would need to be merged by tomorrow. Thanks!

[Tamrat-B]

@ggetz thanks for the updates. We will try to get in the required changes in by today and tomorrow; would be great if you are able to help in updating the release guides or help with unit tests. Thanks!

[ggetz]

@Tamrat-B Great, thank you!

I'll update the release guide based on my understanding of the token rotation process.

For unit tests, let me know what issues you may run into and I'll be happy to provide guidance.

[ggetz]

Thanks for the updates @Tamrat-B and @gowin20. I noticed a couple issues. Would you be able to take a look?

[ggetz]

With this change, this._credit is no longer assigned unless options.warning is defined.This breaks the existing behavior– Now when not using options.warning, the provided options.credit will not show up at all.

I would suggest removing the warning parameter option from the constructor here, and instead in fromBasemapType, pass the credit option when creating the provider.

let credit;
if (defined(defaultTokenCredit)) {
    credit = Credit.clone(defaultTokenCredit);
}
return new ArcGisMapServerImageryProvider({
    ...options,
    url: server,
    token: accessToken,
    credit: credit
  });

Additionally, there should be no need to set this._useTiles in this case. It defaults to true, but you can also set options.usePreCachedTilesIfAvailable to true if you'd like to be explicit.

return new ArcGisMapServerImageryProvider({
    ...options,
    url: server,
    token: accessToken,
    credit: credit,
    usePreCachedTilesIfAvailable: true // ArcGIS Base Map Service Layers only support Tiled views
  });

[ggetz]

@Tamrat-B and @gowin20 We will shortly begin the release process for 1.103. This still needs a bit of work, so I'd expect this to go into next month's release.

[Tamrat-B]

@ggetz thanks for the review; will address the asks and also add the unit tests; I have few additional changes as well. ok on the schedule.

[ggetz]

Hi @Tamrat-B, we just merged a larger-scale PR that affects some of the same areas of the codebase as this one. Another merge from main will be required, but I'm happy to help resolve some of those conflicts if needed.

[Tamrat-B]

@ggetz updated pr based on reveiw and also updated tests to pass (thanks for the help earlier today!). Can you take a look and see if you can resolve the conflicts based on your latest changes... I sent you invite for write access to the repo. thx!

[ggetz]

Thanks @Tamrat-B! I merged in the latest changes. Can you confirm everything is working as intended?

I'm noticing a few 404's for a specific tile when any ArcGIS basemap is set. Is this expected behavior?

[Tamrat-B]

@ggetz Thanks! looks good. we just have to swap out the eval keys with yours when they are ready. But other than that looks good. (the 404s are happening when zooming beyond zoom level (request for tiles that don't exist)..the server could be configured to return empty/blank tiles for those & client app needs to handle it. This was not implemented so for now it's expected behavior.

[ggetz]

Thanks @Tamrat-B and @gowin20 ! I've updated this with our own generated token and added instructions for updating it each release. Looking forward to this going out on May 1st!