LoadWithXhr for node

[jimmyangel]

Added loadWithHttpRequest function which is invoked when LoadWithXhr is called from a node.js application. It uses node http(s).request instead of XMLHttpRequest which is only available in the browser.

I tested using sampleTerrain using a node.js application, which is the main reason I made this change. I do not have an extensive test suite for this functionality under node.js, since sampleTerrain is the only use case I have identified for running Cesium functions in node.js.

Jasmine test result for Core/loadWithXhr: 7881 specs, 0 failures, 6 pending specs.

All responseTypes except for 'json' will return the result buffer. Also, a compressed response converts the result buffer to ArrayBuffer.

[hpinkos]

Thanks for the pull request @jimmyangel! @mramato, do you want to review this?

[mramato]

Yes, but I won't have any bandwidth until next week at the earliest.

Thanks for getting the ball rolling on this @jimmyangel. We need a CLA from you in order to actually review and merge this. See https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md#opening-a-pull-request for details, you can just email us a signed individual CLA. Thanks.

[jimmyangel]

Ok, @hpinkos @mramato, then I should have time to write a couple of test cases as well. However, I see that all your tests are for the browser, so it looks like these tests would be the first ones specifically for node. How do you want you integrate node tests into you test suite?

[jimmyangel]

CLA submitted to [email protected]

[pjcozzi]

Thanks @jimmyangel, we received your CLA.

[pjcozzi]

@mramato what do you suggest as the path forward here?

[mramato]

Thanks for getting this started @jimmyangel, sorry for the slow reply here.

I think having a manual polyfill would be okay if it worked for all types, but the json only limitation makes me hesitant to merge this, it's also going to be hard for us to maintain going forward.

I think a better solution would be to use an existing trusted node module (in this case, I would suggest https://www.npmjs.com/package/xhr2). Basically rather than looking for node, add this to the top of loadWithXhr.js (right after the initial requires)

var XMLHttpRequest_impl;
if(typeof XMLHttpRequest !== 'undefined') {
    try {
        XMLHttpRequest_impl = require('xhr2');
    } catch (ex) {
        console.log('In order to use Cesium.loadWithXhr from Node, please add xhr (https://www.npmjs.com/package/xhr2) as a dependency');
    }
} else{
    XMLHttpRequest_impl = XMLHttpRequest;
}

Then we just use XMLHttpRequest_impl where we currently use XMLHttpRequest/

The reason we try/catch is because we don't want to add a dependency to xhr2 directly to Cesium and force everyone to install it, it's optional just for Node users. I think this strategy of optional dependencies to increase Node compatibility is a decent strategy going forward, but I would like to hear what the other maintainers think.

Thanks again for this and I apologize again for taking so long to reply.

[jimmyangel]

Hi @mramato, thanks for your feedback.

Ok, makes total sense. I will be more than happy to try your suggestion. Let me take a crack at it as soon as I get a chance over the next few days.

[mramato]

Awesome, thanks!

[mramato]

@jimmyangel are you still planning on working on this? If not I may finish it up at our code sprint in a couple of weeks. Thanks.

[jimmyangel]

@mramato I am planning to tackle it in the next few days. Thanks. Ricardo.

[jimmyangel]

Hi @mramato, well, I tried xhr2 and it is not as straightforward as it seems. The reason is that xhr2 does not support gziped content. So rather than bending over backwards to add gzip support into the xhr2 library I would like to try again with the native node implementation I provided in which I included gzip support. I think it would be easier to add XML and Text support later, if needed. Right now it works well for sampleTerrain, which was the main reason for me to do this.

Thoughts?

[jimmyangel]

In addition, xhr2 only supports text, json, and arraybuffer responseTypes, which is pretty close to what I have already (the only exception being text responseType).

How would you suggest to proceed?

[mramato]

What about using the request library? https://github.com/request/request It's one of the most standard libraries in Node and the only reason I didn't suggest it before is because I was hoping for "drop-in" compatibility.

I'll also add that this PR is going to end up conflicting with #3476, but looking at this again reminded me that we need to consider Node support there as well.

[jimmyangel]

Request is a nice library but it seems like a big dependency (overkill?) and it still does not address XML support, so I am not sure what it would buy us over native node.

[jimmyangel]

Hi @mramato,

I would like to suggest that you merge this PR and here is my rationale:

1. It is an improvement over current zero support for node
2. It addresses sampleterrain which one of the key reasons for using Cesium in node
3. It already supports json, ArrayBuffer, gzip, http and https
4. No dependencies

If and when XML support becomes necessary, we could do an incremental improvement to add such support, based on specific use cases -- although it could be tricky (I do not know if we can easily find a browser compatible DOM API for node).

FYI, I have created a library to simplify usage of sampleterrain from node. Right now it depends on this branch of Cesium, which is not ideal. I have seen some desire of using sampleterrain from node in the cesium-dev list and hopefully this utility can be helpful to those folks.

However, if you decide not to merge, I totally understand.

Best regards,

Ricardo

[mramato]

@jimmyangel I appreciated your firm stance on this PR. Originally I thought it was more code than it appears to be in practice (the diff is noisy because of the whitespace). I still haven't ruled out using a third-party module, but your approach might be fine for now.

I promise to look at this at the code sprint we're having next week and hopefully merge it then as well.

[hpinkos]

@mramato I merged in master to this PR. Do you have any more comments here?

[hpinkos]

@mramato what's your opinion on this PR? Do you think this is something we want to merge in?

[chris-cooper]

It would be great to see a solution for this merged!

[jimmyangel]

Hi @hpinkos @mramato, I noticed you guys moved LoadWithXhr around, but it is essentially the same code. With the move to the new Ion terrain server I am going to have to redo this with the latest version of Cesium, as it is very important for my project to be able to do terrain sampling off-line.

Question: are you guys going to support LoadWithXhr for node or should I just go ahead and redo this with a new fork of the latest Cesium release (assuming you never merge)?

Thanks

[chris-cooper]

In case you haven't already done this @jimmyangel we have it in a branch: 1.44...PropellerAero:propeller-patch

[jimmyangel]

Hey @chris-cooper that's great! Thanks a lot!

[mramato]

@jimmyangel I apologize for never getting to this (can't believe it's been almost a year since my last comment). If you open a fresh PR (or reset this one) I will look at it for the next release.

[jimmyangel]

@mramato what about the branch @chris-cooper is talking about above. Is that one going to be merged into a release soon?

[mramato]

@mramato what about the branch @chris-cooper is talking about above. Is that one going to be merged into a release soon?

As far as I know, there isn't a pull request open for that branch, if someone can point me to a PR with the latest code, I can review/merge it.

[jimmyangel]

Ok @mramato , if I don't hear back from @chris-cooper soon I will go ahead and rebase & reapply.

[jimmyangel]

Well @mramato this is turning out to be more complicated than just reapply the code on this branch.

First problem was getAbsoluteUri blowing up because of course 'document' is not defined. I noticed you made some changes but also this was not being called in the original version, so it was not blowing up before.

If I work around that (by making the default value for base equals http://localhost/ -- I think that would be legit since I am running a local node), then it works fine with the old way using new Cesium.CesiumTerrainProvider({url : 'https://assets.agi.com/stk-terrain/world'}), but then I get the deprecation warning.

If I then replace with the new way using Cesium.createWorldTerrain(), I get this error: "An error occurred in "CesiumTerrainProvider": An error occurred while accessing https://assets.cesium.com/1/layer.json."

Looks like you guys are doing something 'different' when using createWorldTerrain(). Is there a way to use the Cesium.CesiumTerrainProvider({url}) with the new terrain model?

Any ideas?

[mramato]

@jimmyangel I'm not sure that changing getAbsoluteUri to use http://localhost is a good idea, but I would have to actually step through the code to fully understand what's going on. If you want to rebase this PR with your changes (minus any getAbsoluteUri changes), I'll debug the problem and see if it's anything obvious.

[jimmyangel]

@mramato never mind, I did some debugging and found out I needed to handle 'text' responseType correctly.

I still needed to deal with getAbsoluteUrlthough because document is not defined in node and we have to handle that condition.

Anyways, I will open a new PR and throw this one away. I tried rebasing but it got messy with all the changes that happened last year so I found it easier to just start over. :)