Documentation issues with 1.42 release

[thw0rted]

I'm updating my Typescript typings (ref #5717 ) based on the docs and found a few minor issues.

• A few things on the new Resource class:
  • I suspect you probably didn't mean to make the instance method retryOnError public; that, or I'm not creative enough to think of when an API user would call it
  • The static post claims to be able to take a "string or object" as its sole parameter but fails if options.data is not defined. This was likely copy/pasted from the fetch static method, because it still says "calls fetch() on it" (not "calls post() on it")
  • This might be intentional but several methods transparently use a method parameter on the options object, even though this param is not defined on the public API. I for one would like to be able to make an OPTIONS or HEAD call against a Resource, if I'm going to use the class anyway.
• KmlDataSource static load doesn't list the ability to use a Resource as data but under the hood it's just making an anonymous instance and calling its load method (which can) -- it should have the exact same docs as the instance method. While you're at it, the instance method still lists a query param with no note about deprecation.
• I'm unclear if GoogleEarthEnterpriseMapsProvider can take a Resource as a constructor option (url) because I see Resource.createIfNeeded(url + path) -- I assume that "(Resource instance) plus (string)" is not what was intended here. Similarly, the get for .url just returns this._url instead of this._resource.url.
• I can't find if any ImageryProvider implementations actually inherit from ImageryProvider. ImageryProvider has various default{channel} attributes that never appear to be used anywhere else?
• The new Plane.projectPointOntoPlane method is missing a return type annotation (it's Cartesian3)
• The showOnScreen option to the Credit constructor is incorrectly listed as a String

Hope you don't mind me dumping everything in one place, but I wasn't sure how best to divide up into separate issues.

[thw0rted]

I've found a more troubling issue that maybe I should open a new ticket for. It looks like the new Resource class treats both templateParameters and queryParameters as a map from key to value. This is correct for templates, but incorrect for query.

The combination of {x}=1 and {x}=2 is either x=1 or x=2, whichever is set last (unless overridden with the "default" property). However, if I combine foo?a=1 and foo?a=2, I shouldn't get either of the source URIs, I should get foo?a=1&a=2. This is not academic; it's how browsers have submitted forms with multiple-select checkboxes (for example) since the beginning of the web. I'm not sure it's currently possible to use this class to submit that sort of form, and I think it might take a breaking API change to make it work correctly.

[thw0rted]

One thing to add to my point about the method parameter in Resource. I'd like to see methods on resource for all the common verbs (put/patch/delete/head/options, at least) or a more generic method that takes a method param. Keep in mind that put and patch would both need a data field also. In the meantime, I guess I'll have to fall back on calling loadWithXhr directly...

[thw0rted]

OK this just became a blocker for me. I had been using loadWithXhr for a long time instead of making my own XHRs or migrating to (browser-native) Fetch API, as my primary means of interacting with my backend REST service. I can't use Cesium to do that anymore, because Resource provides no way to specify the method, and (far more troubling) the existing loadWithXhr implementation has been replaced with a call to Resource's instance fetch... which, of course, overrides the specified options.method. This means any legacy code that passes a value for options.method broke when 1.42 was released.

Maybe this should get its own top-level issue?

[thw0rted]

CC @tfili (probably?)

[hpinkos]

Thanks for reporting this @thw0rted. @mramato or @tfili can answer your questions related to the Resource changes.

For your last 3 bullet points:

I can't find if any ImageryProvider implementations actually inherit from ImageryProvider.

ImageryProvider acts like an interface that all the imagery provider classes adhere to. We don't inherit from it though because there's nothing to inherit. All the functionality is in the individual imagery provider classes.

The new Plane.projectPointOntoPlane method is missing a return type annotation (it's Cartesian3)
The showOnScreen option to the Credit constructor is incorrectly listed as a String

I'll fix these doc errors shortly. Thanks for pointing them out!

[shunter]

Keep in mind that the purpose of loadWithXhr and the new Resource class are intended for getting remote data into the Cesium display. They are not really intended for general-purpose use outside of that. There are plenty of other general-purpose libraries available which you could use instead.

[thw0rted]

Point taken @shunter , and honestly the fetch API is not too far from a drop-in replacement. When I started this project, I used the Cesium load functions because they were already there, and did everything I needed. I don't mind migrating away, but I thought I should make sure the team is aware of the regression -- quietly turning a DELETE call into a GET call could cause trouble for some.

[tfili]

@thw0rted How were you using loadWithXhr to make HEAD/OPTIONS requests? loadWithXhr doesn't return any header information.

[tfili]

@thw0rted Check out the resource-fixes branch. I think it addresses everything you mentioned here in the Resource class along with the issue with clone. The only TODO is the query parameters. I just need to figure out the cleanest solution. It would be trivial to make all parameters with multiple instance just be an array, because the helper function objectToQuery we use to build the URL handles this correctly already. The issue is that this isn't always a desired behavior. For example in a WMS request you have

http://server.com/endpoint?service=WMS&request=GetCapabilities and then create a derived resource with

{
  request: 'GetMap',
  BBOX: ...,
  width: ...,
  height: ...
}

and we end up with

http://server.com/endpoint?service=WMS&request=GetCapabilities&request=GetMap&BBOX=... which is not what you want and may not work.

[thw0rted]

re @hpinkos , I think in my Typescript definitions I'm going to remove the default* properties for ImageryProvider. I don't know the history but they don't appear anywhere else in the code -- I suspect they are vestigial, and should have been removed completely some time ago. Then I can treat it as an interface, like you said, with the guarantee that all the actual provider classes implement it fully.

re @tfili , you are correct that loadWithXhr never returned headers. I used it for PUT and DELETE, but I had to do HEAD/OPTIONS using manual XHR. The new Resource methods look much easier to use!

I looked over the branch and it makes sense to me, but it's still missing two documentation fixes:

• is retryOnError supposed to be public?
• Static post can't take a single (url) string as an argument

As for query params, you're basically implementing a version of URLSearchParams, right? Notably, that class treats the params as an array of key value pairs, because that data structure most closely reflects real-world usage of the searchPart. Of course, you can have more than one value with the same key; you can even have the same key/value more than once. Also, while I don't know of any common services that care about the order of parameters in the searchPart, it's possible per the spec.

So, you could keep storing the queryParams as a map, and sometimes have an array as the value for a certain key, and add functions to manipulate those values (append vs replace, maybe remove?), and hope that the order of your parameters never matters, and that's fine. I don't know much about the Uri library you're passing this to, but if it was me, I'd just add a polyfill for URLSearchParams to the build process and use that directly.

[tfili]

See PR #6211 for the hopefully final changes to fix the Resource class.

The two doc issues should have already been fixed yesterday in that branch. I've added setQueryParameters and appendQueryParameters. addQueryParameters has been deprecated to avoid confusion.

getDerivedResource now takes a parameter preserveQueryParameters, that will not overwrite the parent's query parameters, but instead append to them. Defaults to false, so existing functionality is unchanged by default.

Unfortunately Cesium has never paid attention to the order of parameters, so changes would be needed in a bunch of places outside of this class, and I probably won't be able to get to it right now, since it hasn't been an issue thus far. A URLSearchParams polyfill is a good suggestion that I'll keep in mind. Thanks for your suggestions.

[mramato]

As far as I can tell, everything referenced in the original issue description has been fixed. If there are any small things we missed, please open a new issue for them. Thanks!