Improve support for multiple maps on the same page

[paul121]

As part of the work for integrating farmOS-map with farmOS 2.x I want to improve support for adding multiple maps on the same page. This isn't something that happens very often in farmOS, but for the few places we do have multiple maps we have special logic to prevent data from being duplicated across any additional maps. This is a working solution, but I think some fairly simple changes in farmOS-map would greatly simplify the integration and allow for greater flexibility with multiple maps overall.

Trying to keep the Drupalness out of this issue, but it is the driving force so can't be totally ignored 😄 I'll be adding more of those specifics here: https://www.drupal.org/project/farm/issues/3186641

TLDR; the changes are fairly minimal:

• Change the current functionality that all "registered" behaviors are added to all maps.
• Introduce an additional option.behaviors option when creating map instances.
• Identify map instances in the farmOS.map.instances array based on their target ID (which can be considered unique)
• Create a concept of map-instance specific "settings" that could be reused across behaviors and controls, using the farmOS.map.instances.{id}.settings namespace

Benefits (and some additional ones I discovered while writing this):

• Individual maps can get different behaviors.
• Each map provides its own context to store instance specific "settings" - used by other behaviors/controls when created, or throughout the map's lifetime (maybe this is better referred to as map state)
• Map behaviors can be become re-usable & decoupled from their application context
  • Possibly pull map behaviors from farmOS server into FK

---

The core of the issue comes down to how behaviors are added to maps, and how this affects multiple maps on a page. #

• When a map is created via farmOS.map.create() all behaviors currently present in farmOS.map.behaviors are attached to the map by calling their attach() method:

  farmOS-map/src/main.js

  Line 34 in 065939e

  const behaviors = Object.keys(this.behaviors).map(i => this.behaviors[i]);

• This means that if multiple maps are rendered on a page, they all get the farmOS.map.behaviors added.
• Note that farmOS.map.behaviors may change after the initial page load, specifically in the case of Drupal AJAX, which could add additional JS to the page that populates farmOS.map.behaviors.
• Also note that a Drupal AJAX call might add additional maps, or just "replace" a map on the page. So even though one map is visible, this still results in a new map being rendered, and thus the same behavior. Maps added with AJAX can get additional behaviors, but will always get the previous behaviors on the page as well. (Unless the AJAX call cleaned up farmOS.map.behaviors or something).

A solution to this could be adding a configuration that limits behaviors in the global farmOS.map.behaviors namespace from being added by default; something like behavior.default or behavior.attachByDefault. When creating a new map instance, this boolean could be checked before attaching to the map. To maintain backwards compatibility this could default to true. As long as "conflicting" behaviors are modified to not attach by default, this change alone would allow a solution to the above issues/limitations.

Without additional changes, though, it would be up to external code to add all of the non-default behaviors to a map after creating it. Since external JS code is already required to create map instances this isn't a huge deal - In farmOS, this could be done in the farm_map.js (Drupal) behavior that is responsible for creating map instances. Internal to Drupal/farmOS, maps could specify which behaviors they want, and farm_map.js could add them using instance.addBehavior(name, opts)

Alternatively... I think it would be useful to introduce a new behaviors option when creating a map instance via farmOS.map.create(id, opts). Ideally it could be an array of either behavior names or full behavior objects. Depending on what is provided, the behavior name could be added using instance.addBehavior(name, opts) and objects could be added using isntance.attachBehavior(behavior, opts) here

---

In Drupal, the other aspect to this are the Drupal behavior settings and whether they are "global" settings, or specific to the map "instance". This shouldn't be much of a concern to farmOS-map, but perhaps we could include something in the documentation about this, specifying that behaviors should keep in mind whether they are modifying one or multiple map isntances? As an example, nearly all of our current Drupal behaviors define "global" behavior settings in the Drupal.settings.farm_map.behaviors.{behavior_name} namespace. When these behaviors are attached, they use these variables to change what they do (hence I'm calling them "settings"). A specific example is the WKT behavior:

• It checks if zoom is set, and if so, calls instance.zoomToLayer(layer): https://github.com/farmOS/farmOS/blob/42a8a730ad242c62a40095a011318c5c2a2c62b2/modules/farm/farm_map/js/farmOS.map.behaviors.wkt.js#L38.
• The problem with this is that ALL map instances on the page (that have the WKT behavior) are using the same "setting".

WKT is an exception to this "global" settings pattern, too: it saves the actual WKT data specific to the map instance in a one-off way: https://github.com/farmOS/farmOS/blob/42a8a730ad242c62a40095a011318c5c2a2c62b2/modules/farm/farm_map/js/farmOS.map.behaviors.wkt.js#L6

Again, this is arguably outside the scope of farmOS-map BUT.... what if these values could be saved in the farmOS.map.instances.x.settings namespace? Introducing the above behavior related changes would make this a useful feature for map behaviors in all contexts. The alternative is that map behaviors implement their own instance-specific settings, likely in Drupal.settings.farm_map.instances.{id}. for Drupal map behaviors, which is maybe OK, but also means that a map behavior is coupled to its Drupal environment. If a behavior could use the farmOS.map namespace, the same behavior JS code could be used outside of Drupal, perhaps in FK. Note that this would likely require changing this line to add instances based on their target ID value (and be a general improvement):

farmOS-map/src/main.js

Line 30 in 065939e

this.instances.push(instance);

Taking it a step further, an instance.getSettings()/instance.setSetting() could be introduced on the map instance itself. This would make the settings more structured and allow for behaviors (and controls!) to "share" common settings. An example might be internal the zoom control or measure behavior, a map could provide a default setting disableMeasure = true, which if respected by other behaviors, would prevent the internal measure behavior from being added. (Although thinking out loud, maybe the only "allowed" behaviors would be those specified by options.behaviors when creating the map instance)

---

One thing to note with all of this is that "global" "default" behaviors aren't bad! I don't (totally) hate them ❤️ In farmOS, this is what allows base imagery to be added to ALL maps when a module is enabled. This way the map behaviors are not dependent on any map instance specific settings, they can just do their thing.

For imagery behaviors a common "setting" is the API key used. On one hand, it makes sense for this to be stored in a "global" setting so that it can be used across all map instances. But on the other hand.... is the "duplication" really that big of a deal? Saving an API key within each map instance's settings wouldn't be a huge deal (perhaps the same behavior wants to specify different API keys per map). The advantage of this is that the same behavior code could be used in any farmOS map integration (assuming there is a way for map instance settings to be specified in the application)!

So overall, I see the introduction of map instance settings as being quite useful in that it can help decouple the map behavior JS code from the farmOS-map application context. This might make it possible to pull the same map behavior into FK, similar to how Field Modules work.

[mstenta]

This all makes sense to me. Thanks for the thorough write-up @paul121 !

TLDR; the changes are fairly minimal

Perhaps we could break this up into separate issues and tackle them one at a time, to keep the changes encapsulated and easily understood.

This may also require fixing #84...

Similarly, I don't think the measure behavior would work with multiple maps, for the same reason.

#84 (comment)

[symbioquine]

It's worth noting that I've addressed part of this issue with the farmOS-map 2.x changes to introduce a namedBehaviors namespace where behaviors can be registered without automatically adding them to all subsequently created maps.

[mstenta]

Related: farmOS/farmOS#550