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

New utility object for working with the Cesium ion beta API #6136

Merged
merged 5 commits into from
Jan 19, 2018
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jan 19, 2018

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.

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.
@cesium-concierge
Copy link

Signed CLA is on file.

@mramato, thanks for the pull request! Maintainers, we have a signed CLA from @mramato, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato
Copy link
Contributor Author

mramato commented Jan 19, 2018

CC @tfili @hpinkos

return new CesiumIonResource(options, endpoint, endpointResource);
};

if (defined(Object.create)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What doesn't have Object.create? Older versions of ie?
Should we have a polyfill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

};
}

CesiumIonResource.create = function(endpoint, endpointResource) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should just be a private function that CesiumIon calls, it doesn't need to be a static function on CesiumIonResource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

First inheritance in Cesium!
I know we were hesitant to use inheritance before, is that no longer a concern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't the first inheritance in Cesium, we use it elsewhere (like DeveloperError and RuntimeError) we just use it sparingly.

CesiumIonResource.prototype.createImageryProvider = function() {
var type = this.ionData.endpoint.type;
if (type === 'IMAGERY') {
return createTileMapServiceImageryProvider({ url: this });
Copy link
Contributor

Choose a reason for hiding this comment

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

It always feels weird to me to pass this around in this way. I would do something like createImageryProvider(resource); from CesiumIon.createImageryProvider instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

endpoint: endpoint,
_endpointResource: endpointResource,
_pendingPromise: undefined,
_root: this
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes no sense to me. Anything can do resource.ionData._root can just use resource directly, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).


this.ionData = {
endpoint: endpoint,
_endpointResource: endpointResource,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having this object with private properties, why not just put these private properties on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons:

  1. As I mentioned above, this object is shared across all derived resource.
  2. 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.

// Set the new token for this resource
that.queryParameters.access_token = accessToken;
return true;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

.otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@hpinkos
Copy link
Contributor

hpinkos commented Jan 19, 2018

You have a test failure

@hpinkos
Copy link
Contributor

hpinkos commented Jan 19, 2018

@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.
@mramato
Copy link
Contributor Author

mramato commented Jan 19, 2018

@hpinkos ready, I did end up breaking ionData up because the ion prefix should be good enough anyway. Hopefully the new logic is easier to follow. I also added comments in places that still might be non-obvious

@tfili this also includes the loadWithXhr fixed we discussed offline.

Also fix same requestFunction issue with loadImage and loadJsonp that
loadWithXhr had in my last commit.
@hpinkos
Copy link
Contributor

hpinkos commented Jan 19, 2018

👍 👍 👍 looks a lot better

@hpinkos
Copy link
Contributor

hpinkos commented Jan 19, 2018

@mramato is this ready to go or are you still tinkering with something?

@hpinkos
Copy link
Contributor

hpinkos commented Jan 19, 2018

@tfili do you want to take a look at this or can I merge it?

@mramato
Copy link
Contributor Author

mramato commented Jan 19, 2018

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.

@mramato
Copy link
Contributor Author

mramato commented Jan 19, 2018

I did end up pushing a few more loadXXX fixes that I found while implementing retry for cesium.com, @tfili is doing a review now and we should be good.

Thanks again for the review @hpinkos!

@tfili
Copy link
Contributor

tfili commented Jan 19, 2018

Looks good to me.

@tfili tfili merged commit 6896cd5 into master Jan 19, 2018
@tfili tfili deleted the cesium-ion branch January 19, 2018 22:02
@strech345
Copy link

What is cesium ion? i doesn't find some infos anywhere.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 2, 2018

Hi @strech345! You can find more information about Cesium ion on https://cesium.com/
(note that Cesium ion is the new name for Cesium Composer)

@strech345
Copy link

@hpinkos
ah ok , thanks

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.

5 participants