Support custom tiles source

[HarelM]

This PR should facilitate for feature request describe here: Support custom source #29

I've added documentation above the function itself, I don't know how to convert this to proper documentation.
Any help would be appreciated.

As a side note, since I'm using a patch version of mapbox-gl-js I will only be able to help testing (for example current rc) only after this is merged... Sorry...
I would prefer to have all the breaking changes (mainly name changes etc) as soon as possible since I prefer to work in order to migrate to this library only after the breaking changes had occurred to make sure that everything is working once and I won't need to do it in two steps (first only to migrate to this library and the second to make sure I fix things that were broken assuming I want to upgrade to a later version, which I probably will).
This is of-course only my point of view and needs...

[nyurik]

I am not too sure either way if this is a good approach or not, but just a minor code cleanup suggestion.

[snickell]

I'd like some time to look over this before we consider it, esp since @HarelM can't test directly (thanks for contributing this @HarelM btw!!!), at the very least, this is probably something we should save for "v1.15".

v1.13: full compat
v1.14: shims and renamed internally
v1.15/v2: consider new features

[HarelM]

I'll do the requested changes later on, haven't got to it yet.
I guess a possible solution to allow testing this is to push this into a branch and allow branches to be uploaded to npm as a pre-release, or if you have an alternative that would be great too.
My current solution for allowing this in my code is to use the generated js files, place them in the mapbox-gl folder and run patch-package.
I can obviously do the same here, but I can't qualify this as proper testing... :-)

[nyurik]

I have not dug too deep, but it seems a bit weird to introduce a magical custom source when what we really need is an easy way to register arbitrary protocols -- it should be trivial for the users of the lib to just call mapLibreInstance.registerMyProtocol( 'custom', myCallback ) that would do exactly the same without introducing any magical handling of the custom protocol, plus allow multiple such protocols, plus allow an easy way to override the ones already in the system. I might be missing something though.

[HarelM]

Good point. I'll see what I can do about it.

[HarelM]

Thanks for the review! I've pushed changes according it.
The code around this is fairly simple, there's not much to it beside the changes in the ajax,js file and setting everything up in index.js file, this is what makes it an elegant solution from my point of view.
Let me know if there are other changes required, and let me know when there's a RC with it so I can test it.

[HarelM]

Recent tests shows that the above code is not working as expected since the config object is not defined in the worker context.
The original solution worked since it does not need the config object in the worker context (which is why I think I chose it eventually).
I'll see if I can come up with another solution to this, but I'm not sure I will. If anyone has a thought or idea how to solve this, please let me know...

[AbelVM]

SharedArrayBuffer might be the answer, but... Spectre / Meltdown 😞

[tuukka]

If anyone has a thought or idea how to solve this, please let me know...

If we can't access the list of registered custom schemes here, could we instead check that it's a scheme not handled by fetch? It would just postpone the error message in the rare case of an unhandled scheme a bit. For example:

    if (requestParameters.url.match(/^(\w+):/) &&
        !requestParameters.url.match(/^(about|blob|data|file|ftp|https?):/)
    ) {
        // Use custom handler
    } else {
        // Current logic to use fetch or XMLHttpRequest
    }

[AbelVM]

@HarelM , it looks like the config is accesible from the worker, but the property LOAD_TILES_FUNCTION_MAP has the default value.

Possible reasons I can think of:

1. The serializer is not working as expected (nope)
2. The workers are instantiated as soon as the mapboxgl module is imported, and therefore, the later config changes are ignored. (nope)
3. The workers import the config module, so they will take the default values of the object as declared in the JS file

So lucky number 3. Broadcast Channel API would be great to solve this, but it's not supported by Safari. Workers/Actors code region is a bit obscure to me in order to inject some messaging logic to pass the variables 😞

[HarelM]

I'm guessing sending a function is probably not a good idea, however sending only the keys to allow knowing there's a function in the main thread context is the way to go, IMO.
Having said that, I have no clue how to achieve it yet...
Since I don't expect this to be merged soon I have some time to find out :-)

[AbelVM]

I've been testing that very approach and no change to config, even adding a plain string, is propagated to the workers. They only see the default values. Even using the original getters and setters to set up API_URL or the token... is not propagated to the workers. I have no idea how on earth they consume those params.

Maybe this PR can shed some light on how can we deal with this issue, and, double combo, get rid of custom Mapbox code

[HarelM]

I don't think the config is the right approach for the reasons you mentioned in your comment.
There's a need to call dispatch somewhere to pass data to the workers I believe...
I still need to figure out how...

[hyperknot]

Have you checked out transformRequest? It takes a function which transforms all url's before a fetch by mapbox-gl-js.

[hyperknot]

I'm also using a custom source "maphub:" which I load using transformRequest + a regex. It took about 5 lines to implement it using transformRequest.

[HarelM]

@hyperknot I haven't. If you can share a working example, or something that almost works just to get the idea it would be great. If this is already implemented and there's no need to do anything it would be great :-)

[HarelM]

@hyperknot - this doesn't seem to be a valid solution for reading a tile that is locally stored in a sqlite or indexdb database.
I might be missing out, but I don't see how this solves the case where you don't have network connectivity and would still want to fetch tiles. Please correct me if I'm missing out something obvious...

[hyperknot]

I don't know the internal details of Cordova, etc. but are you saying that a fetch wouldn't work from an url like object (like file:// or whatever)? If so then indeed transformRequest wouldn't work.

[HarelM]

I see. So I guess `transformRequest won't work for my needs. You can think of the following scenario which is not related to Cordova. A user clicks on "Make map available offline" button. In that moment downloading of tiles begin and are stored in indexdb (or wherever). When this operation is finished the user should be able to see the map in the browser without network connectivity. Since running fetch for any url won't help there's a need to be able to inject mapbox-gl with the actual data, stored in IndexDb in this case, so a function to support this is needed.

[AbelVM]

I have a fully working version, should I open a new PR?

[HarelM]

I looked at the code and I think you can send a PR to my repository and I can merge it, but I think more work is needed here to remove the "file or http" assumption...

[HarelM]

There're not much in this PR honestly, a small change to the ajax.js handling for custom protocols.
In any case, I'd appreciate it very much if someone could review and merge this. I've been waiting patiently for a long time...
I saw that @thaddmt just pushed a commit to typescript definition file, so it would be nice if he can review the changes I made to the definition file.

[wipfli]

You are right - it is really frustrating if you have to wait ages before someone even reviews your pull request. One thought that I had was that you could have a look at #105 or #97 such that we can move on with these pull requests and in turn @JannikGM could look at your code.

[bdon]

My 2 cents:

I maintain a fork to implement a very similar feature, but using an async transformRequest as mentioned in this thread above. The downside of the that approach is it's not specific; transformRequest will be called for all HTTP requests, including not only tiles but TileJSON, sprites, and glyph ranges. Also, not all the contexts in which transformRequest is used are asynchronous: example in the code, so implementing it that way would require refactoring a lot more internals.

I tested out this PR by implementing an adapter for PMTiles archives, a COG-like format which fetches raster or vector tiles from a single file via HTTP Range requests.

Here is the code of my implementation. You can run this by putting the file in the debug/ directory of your branch checkout. Demo of how it works:

protocol.mp4

Comments

• Should protocols be specific to tiles and z/x/y or generic to all kinds of resources, including sprites and glyph ranges? Because the current implementation passes an opaque URL string, so you must artificially populate your protocol URL with {z} {x} {y} and then parse it back out with a regex inside your function. Example on line 26. If there was a way to pass Z/X/Y integers in the params, that would be preferred.

• In my example, I prefixed my resource URL with my protocol name like this: pmtiles://https:// - I think the original context was to have a mapbox:// protocol that would assume an HTTPS endpoint, but because this code can't assume HTTP or HTTPS it needs to be awkwardly doubled like this.

• The protocol function as is can return an error state via the callback as described in this docstring:

} else {                       
   callback(new Error(`Tile fetch error: ${t.statusText}`));                   
}

One new condition introduced by protocols is the possibility of missing tiles. For example, if you are fetching tiles from LocalStorage, there might not exist any Tile for a given Z/X/Y, but this is not an error state that should be propagated to the console or other parts of the code. In my implementation I pass an empty array on line 83 for this case, but I'm not sure if this will behave correctly in all cases.

Conclusion

Overall I think this PR is great and minimal, and in my case would discourage me from maintaining a separate fork for the functionality I need, even though the developer ergonomics aren't perfect. It is difficult to predict what protocols others are going to implement, and a more full-fledged API could be evolved from the common pain points shared among Protocols. I think there's some aversion to merging a new feature like this because of the implications of backward compatibility later - maybe it would a good precedent to mark addProtocol as an "EXPERIMENTAL" api that developers should be careful about when upgrading versions?

👍

[AbelVM]

Yay! I made the CI pass. I had to change a bit the regexp to include only urls with ://. @AbelVM please review my changes.
@wipfli feel free to review and merge.
I also changed the typescript definition file to have this methods and changed it to use maplibregl instead of mapbpxgl.

It feels right to me 🙂 We are getting closer to get this functionality onboard! 🎉

Regarding @bdon comments, I think the (minimal) working example I posted at #30 (comment) manages all the bullet points you mentioned 🤓 . Regarding the need of artificially populate your protocol URL with {z} {x} {y}..., just check this PR title: its only target is to provide custom tiled sources, so the source' tiles URL must be as expected by the standard, and then you manage those values within the protocol definition.

[Joxit]

The renaming (mapboxgl to maplibregl) is a bit noisy in this PR, maybe this should be done in a separate PR?

Otherwise LGTM

[HarelM]

If the typescript definition is updated in main I can merge and resolve the conflicts to allow the PR to be less noisy.
Opening another PR, waiting for it to be merged and only then merge this one seems a bit too much (for me...) just for the typings (which were incorrect and I needed to update them anyway).
Regarding experimental suggestion: we can leave it out of the current documentation and see how it goes, if we see that it's good enough as is it can enter or be refactored.
I OK with either approach, I mainly want this in so I can migrate my project.
Once merged, if this can be published as rc I can start progressing my code and project, and probably also ngx-mapbox (which I use) to this org so anyone working with angular (and maybe ionic) can benefit from this as well.
Ping me.
@Joxit Thanks for the review! I appreciate it.

[lseelenbinder]

I like this implementation, but I agree with @bdon that it should be noted that it's experimental and subject to change for a few iterations.

@bdon We could suggest replacing something like pmtiles://https:// with pmtiles+https:// similar to how git uses various transports? Obviously parsing would need to be handled in the function, so it's not ideal. We could also adjust this PR to support <protocol>+<transport>:// and parse out those two details for the callback.

[HarelM]

I'm good with everything you decide, just let me know...
There's also an option to merge this and later on improve this as was suggested here according to developer needs/requirements...
I don't know how to mark is as experimental, so please advise on the proper way (although I think that not documenting it would be the definition of experimental, IMHO).
Seems that everything on the checklist is green, please let me know if and when this is planned to be merged.
Thanks guys for the awesome work!

[wipfli]

Since addProtocol will be part of the public API I would prefer to have documentation for what it does.

[HarelM]

@Joxit I've created the following PR to address the "noise": #172.
Please review it and merge it here so I can carry it downsteam to my repo and reduce the code related to this PR, or simply merge this PR and I'll close the other one. Your call...

[AbelVM]

Looks like merging #172 has tagged this PR as dirty 😞

Once this is fixed, is there any way to help it move forward and be approved?

[HarelM]

I'm planning to fix this tonight unless you @AbelVM want to do it first (I.e. Merge main to branch and resolve conflicts).
I'm not sure what's the latest decision regarding documentation, experimental etc.
I favor experimental, and without documentation at this point.
Once we get some feedback we can decide on our the next steps.
So to summarize, I'm in favor of merging as is and releasing a rc version.
Any objections?

[HarelM]

I've reduced the "noise" last night as requested by @Joxit.
@lseelenbinder I'm not sure how to mark something as experimental, please advise.
Also, please see my previous comment.
I've also sent a message in Slack, I'm planning on merging this tonight (in about 10 hours) if there are not objections...
Worst case this can be reverted as there's not much code involved in this feature...

[Joxit]

Thank you for your changes! 😁
LGTM

[AbelVM]

Hi, @snickell , any chance to green-light this PR soon? 😅

[lseelenbinder]

Thanks @HarelM! I think we can just make a note in the documentation that it's subject to change, but it's also so simple I think it may not really matter.