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

Mapbox Vector Tiles loader #624

Merged
merged 6 commits into from
Jan 13, 2020

Conversation

jesusbotella
Copy link
Contributor

As we talked in visgl/deck.gl#3935, here's the pull request for MapboxVectorTiles loader code. I followed your steps and added some other things for the tests.

I have two doubts about this right now:

  • I have set parse and parseSync methods but the executed code is synchronous as I assumed that the first parameter within the function should be the MVT array buffer. So I don't know if I should remove parseSync as parse will do already.

  • Tests are not passing because I didn't include a loader for MVT. Should I include raw-loader for MVT as it is a binary format? I have never had to require an MVT file through webpack, so I should research that a little.

These are my concerns right now. I'd need to see how load method works so that I can be sure that the attributes passed to parse function are the ones I assumed it would have.

Thank you!

@claassistantio
Copy link

claassistantio commented Jan 13, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

@jesusbotella Thanks, this looks great. I will take over from here.

I have set parse and parseSync methods but the executed code is synchronous as I assumed that the first parameter within the function should be the MVT array buffer. So I don't know if I should remove parseSync as parse will do already.

Correct, parseSync is optional. However, by providing loader.parseSync you enable synchronous parsing using parseSync(arrayBuffer, MVTLoader) which can be nice to have since it does not involve promises etc.

So the reasons for this design is that not all loaders can do synchronous parsing, but it can be simpler to use when available. The reason we still need parse (which can be implemented by calling parseSync when available is that we wanted all loader objects to always have one function, parse.

Tests are not passing because I didn't include a loader for MVT. Should I include raw-loader for MVT as it is a binary format? I have never had to require an MVT file through webpack, so I should research that a little.

Tests should should work, as webpack-dev-server serves the local files during testing. You need to make sure the new module is listed in test/aliases.js

@@ -0,0 +1,3 @@
# Data Attribution

- gadm_usa_3-0-3.mvt from GADM which is free for non-comercial use
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo "commercial"

@ibgreen ibgreen merged commit 95fe0f5 into visgl:master Jan 13, 2020
@jesusbotella jesusbotella deleted the mapbox-vector-tiles-loader branch March 4, 2020 12:11
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.

3 participants