Resource fixes #6211

tfili · 2018-02-13T15:48:46Z

We now handle multiple parameters with the same name.
- addQueryParameters has been replaced with setQueryParameters and appendQueryParameters.
- addTemplateValues has been replaced with setTemplateValues for consistency.
We added static and prototype methods for the other methods
- PUT, PATCH, DELETE
- OPTIONS and HEAD have methods that return a dictionary object of the headers
loadWithXhr has been fixed to actually pass through the method. Even though it is deprecated, this was broken in the last release and can lead to weird bugs if a user was doing anything but a GET.
Resource.clone now functions as expected. Internal logic was moved into the private method Resource.createIfNeeded.
~~server.js was modified to handle OPTIONS requests, so it can be tested.~~
Various doc fixes

cesium-concierge · 2018-02-13T15:48:47Z

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

🌍 🌎 🌏

hpinkos · 2018-02-14T15:39:23Z

Source/Core/Resource.js

+                var value = result[param];
+                var q2Value = q2[param];
+                if (defined(value)) {
+                    if (!Array.isArray(value)) {


Would it make things simpler to have the query properties always be arrays?

Possibly, however we would need to change functionality of objectToQuery and queryToObject to do this. Then a user could still queryParameters property and change it manually, so we'd have to implement a mechanism to prevent that. Since its already done I think it'll be more trouble than its worth.

hpinkos · 2018-02-14T15:44:32Z

Source/Core/Resource.js

+     *
+     *
+     * @example
+     * // Load a single resource asynchronously. In real code, you should use loadBlob instead.


What does In real code, you should use loadBlob instead mean? Is that just a bad copy/paste? It's in the doc for the next few functions too

I think the answer is "both" 😄

Ge means that you wouldn't normally use fetch to retrieve a blob, you would call the more specific fetchBlob instead.

@tfili I would recommend deleting this comment and updating the comment paragraph above to say that we recommend using the more specific functions in the class, i.e. fetchJson, fetchImage, etc.. rather than calling fetch directly, but we expose this for the cases where the user does not want the data to be interested in anyway way. Then just rename blob in the below example to body and we're good to go.

hpinkos · 2018-02-14T15:52:24Z

This all looks good to me @tfili

@mramato do you want to take a quick look too?

mramato · 2018-02-14T15:59:01Z

server.js

@@ -55,7 +55,14 @@
    app.use(function(req, res, next) {
        res.header('Access-Control-Allow-Origin', '*');
        res.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept');
-        next();
+        //intercepts OPTIONS method
+        if ('OPTIONS' === req.method) {


Is this still needed if we're just going to mock OPTIONS in the test? (If not I would remove it since people unfortunately use our code as a basis for their own server).

mramato · 2018-02-14T16:01:59Z

Source/Core/Resource.js

+    /**
+     * @private
+     */
+    function combineQueryParameters(q1, q2, preserveQueryParameters) {


Please add some comments about what this function is actually doing (i.e. perhaps just some examples of expected input/output behavior). The intent of these types of functions are a pain to figure out once you haven't looked at it in a while (or are seeing it for the first time).

mramato · 2018-02-14T16:02:59Z

Source/Core/Resource.js

@@ -239,7 +278,10 @@ define([
     */
    Resource.createIfNeeded = function(resource, options) {
        if (resource instanceof Resource) {
-            return resource.clone();
+            // Keep existing request


Please expand this comment to explain why we are keeping it.

mramato · 2018-02-14T16:10:20Z

Source/Core/Resource.js

+     *
+     *
+     * @example
+     * // Load a single resource asynchronously. In real code, you should use loadBlob instead.


This one is definitely a copy and paste error, remove the commend and change blob to body (I assume DELETE returns the body without interpreting it at all?)

Nevermind, I read the doc, the example should definitely specify a response type (probably JSON) so that users see a real world use case.

mramato · 2018-02-14T17:02:59Z

I think that's all I have. Thanks @tfili

tfili · 2018-02-14T17:39:26Z

@mramato @hpinkos I think that should do it.

mramato · 2018-02-14T21:09:20Z

I resolved the CHANGES conflict.

@hpinkos this is good to me, please merge when you're happy.

hpinkos · 2018-02-14T21:57:48Z

Thanks @tfili!

mramato · 2018-02-15T15:16:29Z

Looks like we missed one, fetchJsonp is still calling addQueryParameters which is deprecated. @tfili can you open a PR with the fix. Thanks.

Tom Fili added 4 commits February 12, 2018 17:09

Some fixes for Resource changes that were reported.

cafc300

Fixed issue with cloning.

b0d80e4

Fixed added HEAD and OPTIONS to the Resource object along with tests.

02a6fc7

More fixes to the Resource class.

7ab9533

tfili mentioned this pull request Feb 13, 2018

Documentation issues with 1.42 release #6205

Closed

Tom Fili added 4 commits February 13, 2018 11:06

Update CHANGES.md

f367db2

Update CHANGES.md

531f13c

Fixed tests.

0d98dab

Merged master.

0ebeb19

tfili mentioned this pull request Feb 13, 2018

We weren't allowing primitive materials to have an image be a Resource #6216

Merged

tfili requested a review from hpinkos February 13, 2018 23:15

Merge branch 'master' into resource-fixes

8f50a76

hpinkos reviewed Feb 14, 2018

View reviewed changes

mramato reviewed Feb 14, 2018

View reviewed changes

Fixed doc.

a36ead7

Merge branch 'master' into resource-fixes

3bf330e

hpinkos merged commit 4e13768 into master Feb 14, 2018

hpinkos deleted the resource-fixes branch February 14, 2018 21:57

tfili mentioned this pull request Feb 15, 2018

Resource clones can't be used at the same time #6208

Closed

hpinkos mentioned this pull request Feb 15, 2018

addQueryParameters -> setQueryParameters #6229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource fixes #6211

Resource fixes #6211

tfili commented Feb 13, 2018 •

edited

cesium-concierge commented Feb 13, 2018

hpinkos Feb 14, 2018

tfili Feb 14, 2018

hpinkos Feb 14, 2018

mramato Feb 14, 2018

hpinkos commented Feb 14, 2018

mramato Feb 14, 2018

mramato Feb 14, 2018

mramato Feb 14, 2018

mramato Feb 14, 2018

mramato Feb 14, 2018

mramato commented Feb 14, 2018

tfili commented Feb 14, 2018

mramato commented Feb 14, 2018

hpinkos commented Feb 14, 2018

mramato commented Feb 15, 2018

Resource fixes #6211

Resource fixes #6211

Conversation

tfili commented Feb 13, 2018 • edited

cesium-concierge commented Feb 13, 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

hpinkos commented Feb 14, 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

mramato commented Feb 14, 2018

tfili commented Feb 14, 2018

mramato commented Feb 14, 2018

hpinkos commented Feb 14, 2018

mramato commented Feb 15, 2018

tfili commented Feb 13, 2018 •

edited