Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add load tile function for a custom source option #10008

Closed
wants to merge 1 commit into from

Conversation

HarelM
Copy link

@HarelM HarelM commented Oct 1, 2020

Basically what I described in: Custom sources #2920.
By adding a custom:// url for tile url in the style and adding this function implementation to mapboxgl.loadTilesFunction this allows the clients to be able to get tiles from local storage, file system, indexDB and other places.
These are all the required code changes, which I believe are relatively small.
I'm not sure what else and where else to document/test/describe this change.

return self.worker.actor.send('getResource', requestParameters, callback);
}
if (/^custom:/.test(requestParameters.url) && !isWorker() && config.LOAD_TILES_FUNCTION != null) {
return config.LOAD_TILES_FUNCTION(requestParameters, callback);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HarelM I'm not an owner of this repo and don't have approval privileges, but I am very interested to see this change go into main. Anyway, this PR is failing in CircleCI. First issue is that the name of this function is causing the linter to fail. The linter doesn't like function names that start with an Uppercase letter, unless the function is a constructor. So LOAD_TILES_FUNCTION should be renamed to something like load_tiles_function or loadTilesFunction.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I have little faith it will be merged... :-( I talked about it in the relevant issue a long time ago, I asked if I should submit a PR and didn't got any answer. I opened this PR as part of Hacktoberfest just to make me feel better that I did everything I could in order to get this in :-)

*/

set loadTilesFunction(loadFn: Function) {
config.LOAD_TILES_FUNCTION = loadFn;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue in CircleCI is that this property is not defined in the Config type in config.js.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the ACCESS_TOKEN pattern, I don't mind changing this, let me know what you think I should use...

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2020

CLA assistant check
All committers have signed the CLA.

@douglasg14b
Copy link

Has there been any movement on this?

@ansis
Copy link
Contributor

ansis commented Mar 10, 2022

I apologize for the silence here.

We see how this can be useful but we're not sure we want to commit to supporting this specific interface. Since we're not actively moving this forward, I'm going to close this pr. Again, sorry for not making this call year(s) ago.

We'll reopen if we look at this again.

@ansis ansis closed this Mar 10, 2022
@HarelM
Copy link
Author

HarelM commented Mar 10, 2022

No worries, I wasn't really expecting it to be merged anyway.
I've added it to MapLibre and I can say it requires little to no maintenance to support this and it brings a ton of new possibilities. :-)
But hey, your call :-)

@douglasg14b
Copy link

douglasg14b commented Mar 10, 2022

@HarelM Mind linking to the Maplibre PR? I'm rather curious there as this is a persistent pain point for some map features I have.

@HarelM
Copy link
Author

HarelM commented Mar 10, 2022

Simply use addProtocol in version 1.15 or above of maplibre. The PR is very similar to this one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants