Moved all low level load functions into Resource #6138

tfili · 2018-01-20T04:24:04Z

Right now all the Resource functions are all private and the load functions just call them.

It also prevents contention issues, since a Resource can only be requested once.

We need to figure out if we're gonna deprecate the load functions or just roll with what we have. We can copy the tests over if we decide to get rid of the load functions.

TODO:

Possibly add some response headers to the Resource, so we can get content-length and stuff like that.
Figure out whats going on with deprecation and tests.

cesium-concierge · 2018-01-20T04:24:05Z

@tfili, thanks for the pull request! Maintainers, we have a signed CLA from @tfili, 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 · 2018-01-22T15:54:58Z

My vote is to deprecate and ultimately remove all of the loadXXX functions. I can't think of a good reason to keep them around, @shunter?

tfili · 2018-01-22T22:22:01Z

So I've deprecated all the functions and replaced their usage within Cesium/Sandcastle.

tfili · 2018-01-23T21:02:52Z

Resolved conflicts. Bump.

CC @mramato @hpinkos @shunter

mramato · 2018-01-25T02:11:02Z

Sorry for not looking at this yet, will try to get to it on Thursday, but might be until Friday.

tfili · 2018-01-26T18:18:25Z

Bump.

tfili · 2018-01-29T18:44:04Z

Another bump.

mramato · 2018-01-29T18:53:09Z

@tfili do you care if we merge this Feb 2nd? Or is there a really good reason to get it into this release? I'll try and review later today either way.

tfili · 2018-01-29T19:14:40Z

We deprecated a bunch of parameters to the load* functions. This removed those and actually deprecated the whole functions. If people were using them, they may find it odd that we did that in back to back releases, but that's probably about it.

mramato · 2018-01-29T19:39:24Z

Avoiding churn is certainly a good reason. Since no one else seems interested in reviewing this, I'll do a thorough review tonight and merge in the morning if nothing major.

…cated.

mramato · 2018-01-29T22:30:05Z

Apps/Sandcastle/CesiumSandcastle.js

@@ -957,11 +958,13 @@ require({
                }
            }
        };
-        return loadWithXhr({
+
+        var resource = new Resource({
            url : 'https://api.github.com/gists',
            data : JSON.stringify(data),
            method : 'POST'


Does method still need to be part of the resource? Wouldn't this just be controlled by the function you are calling? For example, it seems weird to call fetch below, why not just have a post function on Resource?

mramato · 2018-01-29T22:31:33Z

Apps/Sandcastle/gallery/Billboards.html

@@ -144,8 +144,8 @@
 function offsetByDistance() {
    Sandcastle.declare(offsetByDistance);
    Cesium.when.all([
-         Cesium.loadImage('../images/Cesium_Logo_overlay.png'),
-         Cesium.loadImage('../images/facility.gif')
+         Cesium.Resource.createIfNeeded('../images/Cesium_Logo_overlay.png').fetchImage(),


createIfNeeded is private, so it shouldn't be used in Sandcastle examples. To keep these as one-liners, the better approach would be to have a static Resource.fetchImage that just takes the same parameters as the Resource constructor and just returns a promise.

Same comment applies anywhere we create a resource just to load and then throw it away. I originally thought this could wait for a future PR, but I think these examples will get overly verbose if we don't do it now.

mramato · 2018-01-29T22:35:22Z

Apps/Sandcastle/gallery/Custom Geocoder.html

-    var requestString = endpoint + query;
-    return Cesium.loadJson(requestString)
+    var resource = new Cesium.Resource({
+        url: endpoint + query


Shouldn't we use Resource properly and pass in the query parameters separately instead of globbing them together?

mramato · 2018-01-29T22:36:29Z

Source/Core/Resource.js

        if (defined(options.proxy)) {
            resource.proxy = options.proxy;
        }
        if (defined(options.request)) {
            resource.request = options.request;
+        } else {
+            // Clone the resource so we keep all the throttle settings


resource -> request?

mramato · 2018-01-29T22:37:19Z

Source/Core/Resource.js

+         * @memberof Resource.prototype
+         * @type {Object}
+         */
+        lastResponse: {


Did this property exist elsewhere already, or is it new? Are we worried it could lead to bloated memory usage?

The idea was to set the response that is returned so the caller can get information about the response besides just the data. This would make it possible to avoid hacks like #1353. It's not used so I can remove it.

All imagery and terrain providers discard the resource once its received, so we'd probably be ok as far as bloating goes.

mramato · 2018-01-29T22:40:53Z

Since we're going "whole hog" here, I feel like Resource should take either a string or object in its constructor, so that along with the static functions I asked for, we can do Resource.fetchImage('someurl');, otherwise I feel like the API is going to be taking a step back in usability.

mramato · 2018-01-29T22:41:36Z

I did a cursory review and that's all I had. I'll take a closer look once we implement (or discuss) the ideas I brought up.

tfili · 2018-01-30T02:11:09Z

@mramato
I added static methods for all the fetch* methods.
I added a post method on the prototype and a static version.
I let constructor now take a URL or an object.
Cleaned up Sandcastle examples.

Do we want to move all the load* tests into resource as fetch* tests?

mramato · 2018-01-30T13:53:25Z

Source/Core/Resource.js

+     * @param {String} url The url of the resource.
+     * @returns {Promise.<Object>|undefined} a promise that will resolve to the requested data when loaded. Returns undefined if <code>request.throttle</code> is true and the request does not have high enough priority.
+     */
+    Resource.fetchJson = function (url) {


This can be an object or a url, right? We should make sure we document these consistently. My assumption is that the static functions have the same function signature as the constructor, plus any function-specific options, (like post data). We can talk face to face when you get in to avoid PR churn if that helps.

tfili · 2018-01-30T18:34:15Z

@mramato I think this should be good now. Also added tests for the static calls because they aren't tested with the load* tests.

mramato · 2018-01-30T21:06:11Z

Thanks @tfili, I submitted one minor tweak to Sandcastle (just to use the shorthand versions) and I'll merge as soon as this goes green.

mramato · 2018-01-30T21:06:44Z

Actually, one last thing. Update CHANGES and make sure to edit the previous entry regarding proxy et al.

tfili · 2018-01-30T22:20:12Z

@mramato I've update CHANGES

mramato · 2018-01-31T01:31:13Z

Nice work @tfili

Tom Fili added 7 commits January 19, 2018 16:35

Moved loadImage and loadImageViaBlob into Resource.

82f1638

Moved loadArrayBuffer and loadBlob into Resource.

012269b

Some more stuff.

e9f43ea

Merged master.

9d3159f

Moved loadWithXhr into Resource.

7cc6060

Brought over loadJsonp to Resource class.

ece7ad4

Added Request.clone and used it for derived resources.

70e7a71

Deprecated low level load functions.

91d306d

Tom Fili added 2 commits January 23, 2018 14:41

Added ability to get response information from a resource request.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

bb1c18a

Merged in master.

8f7c43d

mramato mentioned this pull request Jan 25, 2018

Don't try to clone FormData #6157

Merged

Merged master.

ec1d86b

eslint fix.

d9f3433

Removed private from functions now that the load* functions are depre…

079a38c

…cated.

mramato reviewed Jan 29, 2018

View reviewed changes

Tom Fili added 2 commits January 29, 2018 21:03

Tweaks from PR comments.

eb19334

Added another test.

7e4e73e

eslint fix.

3d5e9fd

mramato reviewed Jan 30, 2018

View reviewed changes

Added tests and cleanup.

b904c4e

Tom Fili and others added 2 commits January 30, 2018 15:50

Moved lastResponse property and fixed doc.

e93b3b1

Tweak Sandcastle to use less verbose API.

0f7c75b

Tom Fili added 2 commits January 30, 2018 16:12

Updated changes.md

80d5e9b

Merged master.

794e530

Tweak CHANGES

b6378de

mramato merged commit 87716cd into master Jan 31, 2018

mramato deleted the load-functions-into-resource branch January 31, 2018 01:31

hpinkos mentioned this pull request Feb 9, 2018

Specs have deprecation warnings #6202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved all low level load functions into Resource #6138

Moved all low level load functions into Resource #6138

tfili commented Jan 20, 2018 •

edited

Loading

cesium-concierge commented Jan 20, 2018

mramato commented Jan 22, 2018

tfili commented Jan 22, 2018

tfili commented Jan 23, 2018

mramato commented Jan 25, 2018

tfili commented Jan 26, 2018

tfili commented Jan 29, 2018

mramato commented Jan 29, 2018

tfili commented Jan 29, 2018

mramato commented Jan 29, 2018

mramato Jan 29, 2018

mramato Jan 29, 2018

mramato Jan 29, 2018

mramato Jan 29, 2018

mramato Jan 29, 2018

mramato Jan 29, 2018

tfili Jan 30, 2018

mramato commented Jan 29, 2018

mramato commented Jan 29, 2018

tfili commented Jan 30, 2018

mramato Jan 30, 2018

tfili commented Jan 30, 2018

mramato commented Jan 30, 2018

mramato commented Jan 30, 2018

tfili commented Jan 30, 2018

mramato commented Jan 31, 2018

Moved all low level load functions into Resource #6138

Moved all low level load functions into Resource #6138

Conversation

tfili commented Jan 20, 2018 • edited Loading

cesium-concierge commented Jan 20, 2018

mramato commented Jan 22, 2018

tfili commented Jan 22, 2018

tfili commented Jan 23, 2018

mramato commented Jan 25, 2018

tfili commented Jan 26, 2018

tfili commented Jan 29, 2018

mramato commented Jan 29, 2018

tfili commented Jan 29, 2018

mramato commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Jan 29, 2018

mramato commented Jan 29, 2018

tfili commented Jan 30, 2018

Choose a reason for hiding this comment

tfili commented Jan 30, 2018

mramato commented Jan 30, 2018

mramato commented Jan 30, 2018

tfili commented Jan 30, 2018

mramato commented Jan 31, 2018

tfili commented Jan 20, 2018 •

edited

Loading