New utility object for working with the Cesium ion beta API #6136
Conversation
It's marked experimental since ion is still in beta. I also kept the API surface area low since it's easier to add stuff later rather than take things out.
|
@mramato, thanks for the pull request! Maintainers, we have a signed CLA from @mramato, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
| return new CesiumIonResource(options, endpoint, endpointResource); | ||
| }; | ||
|
|
||
| if (defined(Object.create)) { |
There was a problem hiding this comment.
What doesn't have Object.create? Older versions of ie?
Should we have a polyfill?
There was a problem hiding this comment.
This just follows current practices already used elsewhere (agian like RuntimeError and DeveloperError, switching to a different convention is outside the scope of this PR.
Source/Scene/CesiumIon.js
Outdated
| }; | ||
| } | ||
|
|
||
| CesiumIonResource.create = function(endpoint, endpointResource) { |
There was a problem hiding this comment.
I feel like this should just be a private function that CesiumIon calls, it doesn't need to be a static function on CesiumIonResource
There was a problem hiding this comment.
It's needed for testing/mocking (and eventually will probably be made a public API point for when a user already has the endpoint metadata and wants to create a resource synchronously).
| * @private | ||
| */ | ||
| function CesiumIonResource(options, endpoint, endpointResource) { | ||
| Resource.call(this, options); |
There was a problem hiding this comment.
First inheritance in Cesium!
I know we were hesitant to use inheritance before, is that no longer a concern?
There was a problem hiding this comment.
This isn't the first inheritance in Cesium, we use it elsewhere (like DeveloperError and RuntimeError) we just use it sparingly.
Source/Scene/CesiumIon.js
Outdated
| CesiumIonResource.prototype.createImageryProvider = function() { | ||
| var type = this.ionData.endpoint.type; | ||
| if (type === 'IMAGERY') { | ||
| return createTileMapServiceImageryProvider({ url: this }); |
There was a problem hiding this comment.
It always feels weird to me to pass this around in this way. I would do something like createImageryProvider(resource); from CesiumIon.createImageryProvider instead.
There was a problem hiding this comment.
There's nothing weird about it at all. I don't want to move this code into CesiumIon.createImageryProvider because then the CesiumIonResource one won't work on it's own (which is the eventual plan once CesiumIonResource is promoted to a first class public object)
Source/Scene/CesiumIon.js
Outdated
| endpoint: endpoint, | ||
| _endpointResource: endpointResource, | ||
| _pendingPromise: undefined, | ||
| _root: this |
There was a problem hiding this comment.
This makes no sense to me. Anything can do resource.ionData._root can just use resource directly, right?
There was a problem hiding this comment.
No, this object is shared across all derived instances so that a derived instance can modify the token on the root instance. I'll add a comment (I'll also see if I can restructure the code to make it more clear, but probably not).
Source/Scene/CesiumIon.js
Outdated
|
|
||
| this.ionData = { | ||
| endpoint: endpoint, | ||
| _endpointResource: endpointResource, |
There was a problem hiding this comment.
Instead of having this object with private properties, why not just put these private properties on this?
There was a problem hiding this comment.
Two reasons:
- As I mentioned above, this object is shared across all derived resource.
- Since this class derives from
Resource, its better to put everything into a single object to avoid potential collisions if Resource gets a property of the same name added to it.
Source/Scene/CesiumIon.js
Outdated
| // Set the new token for this resource | ||
| that.queryParameters.access_token = accessToken; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
We are returning this promise for the code handling the retry to consume, so it's their job to handle the otherwise (which currently happens in loadWithXhr and loadImage I believe).
|
You have a test failure |
|
@mramato those are all my comments for now |
1. Fix an issue with `loadWithXhr` where a retried request would not have updated settings modified by the retry callback. 2. Clean up specs so that they can't interfere with each other. 3. Break `ionData` into separate properties and simplify some of the logic as well as add additional comments.
Also fix same requestFunction issue with loadImage and loadJsonp that loadWithXhr had in my last commit.
|
👍 👍 👍 looks a lot better |
|
@mramato is this ready to go or are you still tinkering with something? |
|
@tfili do you want to take a look at this or can I merge it? |
|
Unless @tfili has any requests, this is ready to go. I did find some other Resource related issues, but those are ultimately unrelated and can be done in a separate PR. |
|
Looks good to me. |
|
What is cesium ion? i doesn't find some infos anywhere. |
|
Hi @strech345! You can find more information about Cesium ion on https://cesium.com/ |
|
@hpinkos |
It's marked experimental since ion is still in beta. I also kept the API surface area low since it's easier to add stuff later rather than take things out.