mapbox/vtquery

Header-only lib

Opened this issue · 6 comments

mapsam commented

@springmeyer mentioned distributing the key components of vtquery as a header-only library so other tools can use the main query script.

springmeyer commented

👍 low priority, but this ticket can serve as a reminder to circle back on this once the code is working. Basically we should investigate organizing the code such that code depending on other header-only libs is separate from the code that depends on node/nan, as a first step.

mapsam commented

One thing that this would enable is more detailed benchmarks - we could incorporate learnings from hpp-skel as well to test with Google Benchmark, plus running benchmarks in node.

springmeyer commented

Yes. Breaking apart the pure C++ code now, even while stored inside this repo, would enable easier direct unit testing in C++. @mapsam happy to walk you through what this looks like. In short https://github.com/mapbox/route-annotator/blob/master/binding.gyp is a good example. What that does is:

  • Takes all pure C++ code, compiles it into a static library (for best compiling speed. Note: ideally that static library is just the inclusion of the _ipl.hpp per mapbox/hpp-skel#3)
  • Links the node module to that static lib
  • Builds unit tests as another targer
  • For ease of running everything, modifies the Makefile to test both the C++ unit tests and the node module at once: https://github.com/mapbox/route-annotator/blob/master/Makefile#L47
springmeyer commented

Back here to share further thoughts and ideas for next actions. I think the main goal here should be to first separate the code that deals with vector tiles as much as possible from the code that deals with node specific functionality. After that split is in place then it will become more feasible/clear for whether the vector tile specific code can/should live elsewhere. Also, after that split it would become more feasible to add an emscriptem/web-assembly API interface inside vtquery to complement the node interface.

So, what this would look like is:

  • A new .hpp file that consolidates code that does not depend on #include <nan.h>. The src/util.hpp would function as this if the CallbackError code where moved out.
  • The vtquery.cpp getting smaller as a lot of code moves out. For example most of the code inside Execute ideally could be moved into the non-nan header. The gocha here is that code depends on TileObject which has a small dependency on nan/node due to Nan::Persistent<v8::Object> buffer_ref - perhaps this could be templated or otherwise moved off the class to avoid this TileObject having a dependency on node? If it could then the TileObject could be used from a web assembly API...
springmeyer commented

would enable easier direct unit testing in C++

One update on this idea (seen above in the motivations). I no longer think that it is critical that we do any direct unit testing here in pure C++ because:

  • most of the code we would have wanted to directly unit test is now inside https://github.com/mapbox/vector-tile and we should unit test there
  • this repo is now at 98% coverage, so we're solid on coverage just from JS tests.
springmeyer commented

Example of what I have in mind: mapbox/vtvalidate#14