homebay/re-mapbox

Mapbox GL bindings for ReasonML

re-mapbox's Issues

Add better types to Mapbox style expressions

https://docs.mapbox.com/mapbox-gl-js/style-spec/#expression-reference

Using this API via an array of JSON is pretty error-prone. I started adding an ADT for Expressions, but it's very incomplete (basically "get", raw expression array and raw value). This could be so much better:

• Change to a gadt, with typed return values from expressions
• Add "has", which always returns a bool
• Confirm that nothing is using RawValue and remove it
• Add "literal" instead, and make it work correctly
• The type of value passed to Layer's filter field should be of type Expression.t(bool)

FFI Structure

Currently, the Mapbox FFI is a mix of zero-runtime-cost bindings to raw Mapbox types/functions and a layer of Reason types with runtime conversion to Mapbox types. I probably won't knock all of this out at once, but here's a plan to clean things up:

• Move GeoJSON out of this repo
• Inside src/ make two directories
  • FFI (filled with low-level abstract types)
  • Re ( filled with Reason wrappers and runtime conversions)

The low-level bindings in src/FFI/ should provide as much safety as possible while bringing virtually no runtime cost. We should generate *.rei interfaces here and aggressively hide anything unsafe.

Examples of strategies in FFI:

• [@bs.deriving abstract] all over the place
• Obj.magic to cast types to their Mapbox supertypes
• = "%identity" to constrain strings, emulating enums

The src/Re/ directory should have 1:1 mappings to files in the FFI directory (mostly, probably?), but all objects should be Reason records, all subtyping should be represented as variants, and all functions that call into the FFI should do runtime conversion as needed.

This approach should make it clear when you're choosing between clean JS code vs real Reason data that can be pattern-matched and updated using normal patterns (but with more overhead in the compiled JS).

Inline literal string enums

There are a few places the Mapbox API specifies a finite number of allowable strings to be used as options. I think we've mostly taken the approach of defining an abstract type and using "%identity" to cast valid strings as that type. It has worked, but this article has a couple great tips that can improve things:

• By pairing a module type with a module, we can use type t = string but hide the details from the outside world, so it still works the same, but there's no need for "%identity" hacks and separating interface files
• By adding [@bs.inline] annotations, we can avoid defining string variants that end up unused

Switch to bs.obj

We probably can't immediately take advantage of Bucklescript v7's records-as-js-objects since so many of the Mapbox objects expect fields to be missing (instead of undefined values), but we could consider switching from [@bs.deriving abstract] to a bs.obj implementation.

This has the advantage of being slightly more straightforward to read than the magical derived functions that come from @bs.deriving abstract, plus for simply constructing objects, we don't actually need any of the generated getter functions.