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

Make Node Map object options "request" property optional #904

Merged
merged 5 commits into from
Mar 23, 2023

Conversation

tdcosta100
Copy link
Collaborator

The current Node Map requires you to implement a request function, so everything Node Map needs, it asks to this function. This PR allows you to choose if you really want this, or if it's ok to let Node Map use its internal loaders.

@tdcosta100 tdcosta100 force-pushed the nodemap-request-optional branch from dcd9dde to 7707b80 Compare March 16, 2023 03:36
@louwers louwers added the node label Mar 16, 2023
@acalcutt
Copy link
Collaborator

acalcutt commented Mar 16, 2023

What's the difference between the internal loaders and the one you implement yourself? Would it work with http and local files or would it depend what you wanted to use this with if you need to implement your own. What do these default options work with?

Based on the documentation at https://github.com/maplibre/maplibre-gl-native/tree/main/platform/node , the implementation of options is different if you are doing local file vs https request.

@tdcosta100
Copy link
Collaborator Author

tdcosta100 commented Mar 17, 2023

The internal loaders implement their own logic, you can learn about them here. But they work well for the basics. There is a provider for local files (local_file_source.cpp), or online files (http_file_source.cpp), and a dedicated provider for MBTiles as well (mbtiles_file_source.cpp). So unless they don't do something specific you need, they are a good fit, so there's no point in reimplementing something they already do, like dealing with all those HTTP status codes and stuff. But of course, if your logic is more complex, like doing specific things depending on the resource requested, you still can implement your request function.

@tdcosta100
Copy link
Collaborator Author

An addendum: if you implement your request function, then the internal loaders are disabled and all requests are sent to your function, exactly like it was before this PR.

@acalcutt acalcutt force-pushed the nodemap-request-optional branch from 7707b80 to 3319c0d Compare March 19, 2023 13:41
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Typescript index.d.ts needs to be updated to reflect this.

@tdcosta100
Copy link
Collaborator Author

Done!

@louwers louwers self-requested a review March 19, 2023 23:48
@tdcosta100 tdcosta100 force-pushed the nodemap-request-optional branch from d1a6fc1 to a422920 Compare March 21, 2023 08:11
@acalcutt acalcutt force-pushed the nodemap-request-optional branch from a422920 to 5729ec9 Compare March 22, 2023 21:24
@louwers louwers requested a review from acalcutt March 22, 2023 21:30
Copy link
Collaborator

@acalcutt acalcutt left a comment

Choose a reason for hiding this comment

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

I tested this and it seems to work. I was able to use options like was required in the past, or go without the options all together.

Can you add an entry to the node readme for this.

@acalcutt acalcutt force-pushed the nodemap-request-optional branch from 5729ec9 to 073d741 Compare March 23, 2023 03:45
@tdcosta100
Copy link
Collaborator Author

Nice. Please check if the README.md changes are reflecting all those changes.

@acalcutt acalcutt dismissed louwers’s stale review March 23, 2023 12:55

typings have been added and tested

@acalcutt acalcutt merged commit ee44990 into maplibre:main Mar 23, 2023
@tdcosta100 tdcosta100 deleted the nodemap-request-optional branch March 23, 2023 20:43
tdcosta100 added a commit to tdcosta100/maplibre-native that referenced this pull request Mar 23, 2023
tdcosta100 added a commit to tdcosta100/maplibre-native that referenced this pull request Mar 23, 2023
tdcosta100 added a commit to tdcosta100/maplibre-native that referenced this pull request Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants