Unclear requirements for GeoJSON to be provided in addFeatures

[nerik]

Using addFeatures, validation fails if features:

• don't have an id set with a proper UUID string
• don't have a mode set in the feature's properties.

This behavior might be problematic for a few different reasons:

• both of those requirements are fairly internal to Terra Draw, so needing to have them when for instance, preload TerraDraw with an existing GeoJSON document is a bit disorientating and unpractical. Would you consider setting default values?
• ~it's unclear from the docs what is required~ missed that bit in the docs, sorry https://github.com/JamesLMilner/terra-draw/blob/main/guides/4.MODES.md#loading-features
• unless I'm mistaken, it's also unclear from the TS type (type GeoJSONStoreFeatures = Feature<GeoJSONStoreGeometries, DefinedProperties>;)

Not sure if/how you want to see these things address but happy to give a hand. Cheers!

[JamesLMilner]

Hey @nerik thanks for raising this, appreciate the feedback about this! Agree these requirements could be made a little bit clearer. I was thinking about it yesterday and wondered if maybe we could make the addFeatures method a little more forgiving and maybe also improve the documentation around it.

There are some considerations around this however - imagine a user adds a series of Polygons to their Terra Draw instance, we don't necessarily know if that should be associated with TerraDrawPolygonMode or one of the users custom drawing modes that they may have created. Additionally, we would have to consider what to do if the default TerraDrawPolygonMode wasn't instantiated with the Terra Draw instance at all. This is why I went with the explicit option, because although slightly more awkward, it forces the user to describe exactly what they would like to happen with the data they are adding.

This being said I agree it's not super user friendly and maybe there's a 'sensible default' that would work for 80% of peoples use cases, which I imagine would just be adding and associating Polygons to the TerraDrawPolygonMode in the example describe above.

I might experiment with this today and see if there's an approach that feels right!

[JamesLMilner]

Hey @nerik - I was looking into this more deeply today.

I noticed the first point about IDs will fail if there is an existing id provided - which made me think you probably have IDs which are not UUIDv4. I have raised a PR which opens up the types of IDs that you can use with Terra Draw, so if users have features that have a different ID strategy then you can bring your own. Here's an example if you wanted to use incrementing integers:

const draw = new TerraDraw({
  adapter,
  modes,
  idStrategy: {
    isValidId: (id) => typeof id === "number" && Number.isInteger(id),
    getId: (function () {
      let id = 0;
      return function () {
        return ++id;
      };
    })() // Returns a function that returns incrementing integer ids
  },
});

This way you can provide features with any id type the user likes, and it will also generate new ids using the getId function the user provides.

draw.addFeatures([
  {
    type: "Feature",
    geometry: {
      type: "Point",
      coordinates: [-1.825859, 51.178867],
    },
    properties: {
      mode: "point",
    },
  },
]);

The PR also exposes a getFeatureId method, which defaults to return uuid4 or returns whatever the next value is from your idStrategy, if you want to create a feature manually outside Terra Draw.

This gives the developer more control over ids and id validation which was in your first point. With regards to the second point, I think at least for now making users explicitly provide the mode they want to assign the feature to feels right, given the many ways users could choose to use Terra Draw. Hopefully the documentation around this is clear enough and if not would totally welcome feedback on improving it. I appreciate this is slightly more long winded, but I think the trade off is worth it to make sure features end up associate with their correct modes.

[nerik]

Ah, awesome, thanks. Shorter ids are definitely a win in scenarios when data is stored in the URL 🎉 As per the point about mode: while I can follow your rationale, I still stand by my point that in a scenario where you load a GeoJSON from outside the library, one would assume that setting mode and ids would be low-level work done by the library. How would you feel about adding a addFeaturesFromGeoJSON method that would assume defaults, with a "use at your own risk kind of approach?