farmOS-map 2.x Tracking Issue

[symbioquine]

I'm going to use this issue to track the sequence of pull requests that will make up the initial farmOS-map 2.x changes.

Introduction

Why Do We Need a farmOS-map 2.x?

There are a few breaking changes which are needed to allow farmOS-map's functionality and contrib ecosystem to keep growing without a correspondingly growing performance footprint.

What do these changes do?

• All farmOS-map behaviors are changed to being loaded asynchronously.
  This allows their code to be lazy loaded at runtime.
• Behavior loading has been simplified to make it work the same for first-party and contrib behaviors:
  • Remove the concept of behavior "defaults" in favor of the existing farmOS.map.behaviors
    Now all "default" behaviors are loaded from farmOS.map.behaviors when an instance is created.
  • Expose "named behaviors" as farmOS.map.namedBehaviors
    Now the available behaviors which can be attached via instance.addBehavior can be modified/extended instead of just being hard-coded to [rememberLayer, edit, measure, snappingGrid]. This functions as a registry where behaviors can be registered without necessarily having them be loaded on instance creation. That opens up exciting possibilities for patterns where one contrib module can expose a behavior that is leveraged by other behaviors.
• The build is changed from being one monolithic bundle to being composed of a number of lazily loaded "chunks"
  This allows the included behaviors in farmOS-map to keep growing, but not incur a performance penalty unless those behaviors are actually used on/in a given page/app.
• The CSS for farmOS-map is no-longer bundled into the farmOS-map JS - instead there is a top-level farmOS-map.css which must be included in any page using farmOS-map. Lazy loaded functionality automatically includes any additional css as needed.
  This allows pages/apps to directly manage how they consume farmOS-map.css - possibly applying their own minification/aggregation/customization.

Pull Requests

1.x Baseline

• ✅ Merged: Create a test harness for benchmarking different integration examples #112 - This is actually a PR for 1.x, but I'm including it since it provides the underlying test harness I've used to validate the performance of the 2.x changes

2.x

• ✅ Merged: 2.x part #1 - Bump version to 2.0.0-alpha.0 as a baseline to build 2.x changes on #114 - Bump version to 2.0.0-alpha.0 as a baseline to build 2.x changes on.
• ✅ Merged: 2.x part #2 - Split existing behaviors into lazily loaded chunks #116
  • Simplify default behavior loading so it works the same for core and contrib behaviors.
  • Expose "named behaviors" as farmOS.map.namedBehaviors.
  • Make behaviors load asynchronously from separate lazily loaded chunks.
• ✅ Merged: 2.x part #3 - Decouple MapInstanceManager from window.farmOS.map #117 - Decouple MapInstanceManager from window.farmOS.map as described in farmOS-map packaging and module exports #113.
• ✅ Merged: 2.x part #4 - Extract CSS instead of bundling it #118 - Extract CSS instead of bundling it.
• ✅ Merged: 2.x part #5 - Remove core Google maps support #119 - Remove core Google maps support.
• ✅ Merged: 2.x part #6 - Update readme to include upgrade instructions  #120 - Document how to update code that uses farmOS-map from 1.x to 2.x.
• ✅ Merged: 2.x Upgrade to Webpack 5 #121 - Update to Webpack 5 to take advantage of Automatic Public Path support for chunk loading.
• ✅ Merged: Expose MapInstanceManager and the default projection settings at window.farmOS.map #122 - Expose MapInstanceManager and the default projection settings at window.farmOS.map
• ⌛ Pending Review: Upgrade to farmOS-map 2.x farmOS#425 - Update farmOS 2.x to use farmOS-map 2.x
• ✅ Done: Update farm_map_google behavior contrib module for farmOS-map 2.x paul121/farm_map_google#1 - Update the farm_map_google package for 2.x Release updated farm_map_google contrib module - See https://github.com/symbioquine/farm_map_google & https://www.drupal.org/project/farm_map_google
  • ✅ Merged: Use duck-typing for recursing into layer groups mstenta/ol3-google-maps#1 - Fix layer group recursion to work on separately bundled LayerGroup instances.
  • ✅ Merged: @mstenta @symbioquine needs to push the above change up to the NPM package as a new version .e.g. 1.7.1
• 🛠 TODO: Run performance tests automatically with a Github Action
• 🛠 TODO: Publish examples to Github pages with a Github Action

Note: Many of the above changes are the same ones I previously published in #109, but I'm going to republish them now as separate pull requests using this issue to coordinate/track the review/merge process.

[symbioquine]

TODO: Open Issue about browser compatibility in farmOS repo.

[symbioquine]

TODO: Open Issue about browser compatibility in farmOS repo.

Done: https://www.drupal.org/project/farm/issues/3217264

[symbioquine]

@mstenta I've addressed all the points of feedback we came up with during today's dev call. Let me know if you think of anything else that should happen before these can be merged.

[mstenta]

Thanks @symbioquine ! LGTM!

[symbioquine]

@paul121 @jgaehring and I had some really good discussion around this and some of the related/future work which I'll try to summarize here;

---

Short term next steps;

• Tag a 2.0.0-alpha.0 release for farmOS-map @symbioquine done
• Update the farmOS 2.x integration PR to point at that tag @symbioquine done
• Finish updating the Google maps contrib module for farmOS-map 2.x
  • Update farm_map_google behavior contrib module for farmOS-map 2.x paul121/farm_map_google#1 (few prerequisites still outstanding + discussion of moving farm_map_google under the farmOS org

Bigger Picture Stuff

FieldKit Integration of farmOS-map 2.x

@jgaehring will experiment with this in the not-to-distant future.

@symbioquine provided example of earlier prototype of change to do integration #68 (comment) (Some additional changes will make sense since the imports can be a bit cleaner now and it would probably make sense for FieldKit to use the MapInstanceManager class.)

NPM/Webpack integration instructions: https://github.com/farmOS/farmOS-map/tree/2.x#working-with-farmos-map-in-an-npmwebpack-project

Upgrading from 1.x: https://github.com/farmOS/farmOS-map/tree/2.x#upgrading-from-farmos-map-1x-to-2x

Cross-domain farmOS-map caching/versions and FieldKit hosting

One potential downside of bundling farmOS-map with FieldKit is that users will sometimes end up loading farmOS-map twice - once from FieldKit and once if they visit farmOS directly.

If FieldKit were self-hosted it would allow the farmOS-map artifacts to be shared between FieldKit and the main farmOS pages. Also, as @paul121 points out, it could have helped avoid some of the major version upgrade challenges that have been present in moving users to farmOS 2.x at the farmos.app domain.

It's also worth noting that it could be problematic if FieldKit and farmOS were running significantly different versions of farmOS-map since that might break behaviors if those were being loaded from the farmOS server.

farmOS Hosted Client Modules in FieldKit

@paul121 Walked @jgaehring and @symbioquine through the changes to support FieldKit loading "client modules" A.K.A "field modules" from a farmOS 2.x server. The changes are linked from https://www.drupal.org/project/farm/issues/3191246#comment-14124923

Basically, there's a controller which serves the contents of the module's single JS file at farm/client/js/{client_module}/index.js. Since config entities are used to describe the available client modules, FieldKit should be able to query those using the standard JSON API and then use the controller to load/execute the module's JS code.

We discussed how this might violate developer's assumptions that they can declare dependencies for their client module JS using the normal .libraries.yml patterns and would also break cases where the loaded JS expects to be able to dynamically load additional JS from the same path - as in the case of Webpack chunk loading.

@symbioquine suggested that an alternative strategy could be returning an HTML fragment to FieldKit in the same way Drupal/farmOS responds to normal AJAX requests. FieldKit would just append the response HTML fragment to its body - perhaps after some validation. In this way, the controller could use the more standard element rendering and JS/CSS attaching machinery in Drupal which would allow library dependencies and aggregation to work automatically out-of-the-box.

farmOS Map Behaviors and FieldKit

A large part of our discussion was around how farmOS-map behaviors should affect FieldKit. @symbioquine described why it seems pretty obvious that contrib modules in farmOS need to be loaded in FieldKit; Take for instance a farm/garden in a region that doesn't have good Google/MapBox base layers, it should be trivial for the same contrib module to supply custom base layers both for farmOS and FieldKit. Similarly, extending the drawing/geolocation/navigation/accessibility features of the farmOS map, should almost always also extend the map in FieldKit.

We discussed various strategies for making that work - some of which @paul121 has already summarized in farmOS/farmOS#426 (comment)

[symbioquine]

• https://github.com/symbioquine/farm_map_google / https://www.drupal.org/project/farm_map_google

[mstenta]

Are we ready to tag v2.0.0?

Or are there other "breaking changes" that we want to consider before that?

Quick skim of open issues - @symbioquine @paul121 should any of these block v2.0.0?

• Improve support for multiple maps on the same page #94
• Map instance and behavior settings #110

[paul121]

I don't think these need to block 2.0. It seems like the concept of behavior settings could be implemented independent of farmOS-map as is. Later it could be included moved to farmOS-map if these patterns seemed useful in either farmOS-map core behaviors, or adding behaviors to farmOS-map in a decoupled context.

[symbioquine]

Agreed! I think we can resolve this and tag v2.0.0

[mstenta]

Great!

One last thing we should consider... if we do move to using composer-merge-plugin (https://www.drupal.org/project/farm/issues/3230933) instead of asset-packagist for pulling farmOS-map into farmOS, then we can also tweak our NPM packaging script for a v2.0.0 release. (This could be done in a follow-up too, but might as well do it now IMO.)

Currently we are running npm install and npm run build before publishing to NPM. The reason we did this was so that farmOS could use Asset Packagist to pull farmOS from NPM, and have a pre-built copy of farmOS-map.js available in the dist directory.

If we aren't using Asset Packagist anymore, we don't need to do this. So we can probably rearrange the action steps such that it publishes to NPM before running npm install and npm run build. This would also allow us to remove /npm_modules/ from npmignore, I think:

farmOS-map/.npmignore

Line 1 in 9423f71

/node_modules/

[symbioquine]

Currently we are running npm install and npm run build before publishing to NPM. The reason we did this was so that farmOS could use Asset Packagist to pull farmOS from NPM, and have a pre-built copy of farmOS-map.js available in the dist directory.

If we aren't using Asset Packagist anymore, we don't need to do this. So we can probably rearrange the action steps such that it publishes to NPM before running npm install and npm run build.

I think we should still include the built artifacts in the dist/ directory when publishing to NPM.

• From a quick survey of some popular NPM packages, I couldn't find any that have a build step, but aren't executing it prior to publishing. That matches my understanding that it is a pretty strong convention to provide the build artifacts as part of the NPM package.
• Publishing the build artifacts allows for deployment use-cases where NPM package takes a dev dependency on farmOS-map so it can copy the built artifacts and serve them directly - as opposed to needing to rebuild/bundle farmOS-map again or manually manage a reference to the Github release tarball.

[mstenta]

it is a pretty strong convention to provide the build artifacts as part of the NPM package.

Oh ok! If that's a common convention, then we can keep that. Sounds good to me.

@symbioquine do you want to do the honors of releasing v2.0.0? :-)

[symbioquine]

@symbioquine do you want to do the honors of releasing v2.0.0? :-)

Sure :)

[symbioquine]

I've pushed the v2.0.0 release tag - resolving this issue.

@mstenta Maybe you can change the default branch to 2.x now - I don't think I have permission to do that...

[symbioquine]

Actually, @paul121 I just noticed that the built JS/CSS chunks aren't being included... We'll probably need to fix that and tag a v2.0.1 release :(

[symbioquine]

I think #130 should fix it... @paul121 Let me know what you think.

[symbioquine]

The v2.0.1 release includes #130 and I've confirmed that the release zip looks like it has all the expected files now.