Enable use of custom XHR loader for TileVector sources

[ghost]

Currently it's possible to fully customize only the plain Vector source. The TileVector source forces the use of the built in XHR implementation which in turn is coupled to the Format API which is not extensible outside OpenLayers.

The use case is when one wants to use external custom data sources with custom format (see #3754 and #3756) without modifying and custom building OpenLayers.

[ahocevar]

I have concerns about this from an API perspective. Wouldn't it be nicer to have a TileLoadFunction like for tile sources? Could be something like this:

/**
 * A function that is called with a tile url for the features to load and
 * a callback that takes the loaded features as argument.
 *
 * @typedef {function(string, function(Array.<ol.Feature>))}
 * @api
 */
ol.TileVectorLoadFunctionType;

Then your custom loader could look like

function(url, callback) {
  var xhr = new XMLHttpRequest();
  xhr.open(url, true);
  xhr.onload = function() {
    callback(new ol.format.GeoJSON().readFeatures(xhr.responseText));
  }
  xhr.send('');
};

[probins]

I have been meaning to raise a similar issue. The process for loading vector sources currently is:

1. fetch data
2. convert data to features
3. add features to source/layer

But I have several problems with how this currently works:

• no ability to use data that is not in a ol-supported format (same problem as in this ticket)
• not able to add features using the loader, common when reading from database: fetch ?type=restaurant initially, then add ?type=hotel
• not able to write back to the same server, as the url/format/projection are not saved

I can do this with a custom loader and by storing the url/format/projection in my app, but if I do this for some sources, I might as well do it for all, in which case the ol-supplied loader isn't used.

[probins]

plus another use case is to add features loaded from a different source; again I can do this with a custom loader, but the basic issue is that ol.source assumes a one-time read-only source, which isn't always the case with vectors.

[ghost]

@ahocevar, I agree your proposal looks more clean and appropriate. I'll rewrite this PR to match when possible.

@probins, what other sources than Vector (already has custom loader) and TileVector do you think are relevant?

[probins]

ISTM it would be better to pass a convert-data function to the loader, which could be format.readFeatures or could be any other conversion. It isn't the loading that needs to be customised; it's the conversion into features. These aren't necessarily 'custom formats'. You can easily use for example a MongoDB or Google Fusion Tables for storing features in the cloud. The geometries here are GeoJSON but they are not GeoJSON feature collections, and have to be rearranged to create OL features. I've been experimenting with Geobuf, which is similarly based on GeoJSON but not readable with format.GeoJSON.

[ghost]

Currently loading does need to be customised for binary formats (the current xhr-stuff in OpenLayers only supports text based xhr). Also, you might want to load from multiple sources (geometry from one source, attributes from another). So I think pure data conversion is too limited and would like to go with the proposal from @ahocevar.

[probins]

the current xhr loader could be changed to handle non-text responses as well, no? I've not come across the need to load geometry and attributes from separate sources - agreed, you would need a custom loader for that 😃

As you say above, the loader is currently coupled with ol.format, and ISTM it would be better and more flexible if they were uncoupled. I should be able to pass any callback function to xhr to be run onload but before addFeatures.

[ahocevar]

The nice thing about my proposal is that you don't even need xhr. You could also use JSONP or whatever. The app would be responsible for creating a features array (which is easy with the formats available, and possible with whatever your format is if you have application code for it), and once the features are available all the app needs to do is call a callback.

[ahocevar]

@probins

I should be able to pass any callback function to xhr to be run onload but before addFeatures.

This is achieved with my proposal. Maybe you misread it?

[probins]

@ahocevar Are you proposing something similar for source.Vector as well?

[ahocevar]

@probins You can achieve the same with ol.source.Vector, by configuring a loader instead of a url and a format. Look at the vector-osm example to see how this is done.

The only difference is that you call addFeatures instead of a callback that is passed to a loader. So maybe both ol.source.Vector and ol.source.TileVector should work the same - either both with a callback, or both with manually calling addFeatures.

Edit: well, another difference is that my proposal has a function that gets the url already from a urlfunction, whereas the loader in ol.source.Vector has to create the url from extent and resolution.

[probins]

yes, that's what I was meaning - Vector and TileVector should work in the same (or very similar) way.

[ahocevar]

Right. So we can either remove the tileUrlFunction option from ol.source.TileVector, or add a featureUrlFunction option to ol.source.Vector. I think the latter would be more convenient, and it would be more in line with what we have for image and tile sources.

[ghost]

So I've changed the implementation to add TileVectorLoadFunctionType like the proposal from @ahocevar. Can we keep this PR about that and add featureUrlFunction to ol.source.Vector in another?

[ahocevar]

Sure, thanks @sweco-sebhar. I'll review the updated pull request tomorrow. ol.source.Vector can be changed separately of course.

[ahocevar]

The name of the option can be just tileLoadFunction.

[ghost]

Thanks for the review @ahocevar. Name changed to tileLoadFunction and format is now documented and asserted to be required only when there is no tileLoadFunction.

[ahocevar]

Should be

this.format_ = goog.isDef(options.format) ? options.format : null;

[ahocevar]

Thanks for the updates @sweco-sebhar. I made a few more comments. When addressed, I'm going to merge this.

[elemoine]

I think it would be good to add a bit more context here.

[ghost]

Thanks for the input!

I've made changes from your comments @ahocevar. I had to introduce ol.TileVectorLoadFunction.nullTileLoadFunction to make closure compiler happy, similar to ol.TileUrlFunction.nullTileUrlFunction.

I've also tried to elaborate a bit more on the purpose of the tileLoadFunction option as pointed out by @elemoine.

[ahocevar]

Your last change breaks the logic. Please consider 3bacbfb.

[ghost]

Sorry I did not realize that it would break and I did not test. Picked your changes into a new sqashed commit.

[ghost]

Also changed the externs to match the new type definition and have now tested locally by by modifying the tile-vector example.

[ghost]

make ci reveals further type issues which I do not understand yet but will try to fix.

[ahocevar]

olx.js was correct already. There we want either undefined or the function. But for the member in ol.source.TileVector, we want null or the function. The type issues should go away when you change the type in olx.js back to ol.TileVectorLoadFunctionType|undefined.

A short unit test would be nice of course.

[ghost]

Yes came to the same conclusion and have reverted the changes. I lack the experience with closure to avoid the pitfalls. I'm not sure I can produce a unit test in a short time frame but will understand if you want it in place before merge.

[ahocevar]

No it's ok. Your change is quite trivial in the end. Thanks for your work on it, I'm going to merge now.

[ahocevar]

Quick note: I think it would be good if the TileVectorLoadFunction also received the projection as argument.