Issues with dev environment and consuming Node packages

[jgaehring]

I've been trying to set up a development environment for adding new controls, behaviors, etc, either as a third-party extension and/or to include in the official repo.

First I tried to simply clone the repo and branch off 2.x to experiment a bit with the examples. To start off, I found the README a bit confusing in the way it describes using the install-all script "in each example project", when the install-all script is not included in each example, but at the root of the examples directory itself. It also doesn't mention that the examples depend upon the distributable version of the library, which needs to be built first.

Those are really minor issues, but ultimately, it all seemed to be in vain, b/c most of the examples require Mapbox API keys to work. I really don't know if the examples are at all intended for this purpose, but if so, it might be helpful to include an OpenStreetMaps example that's a little more sophisticated than the minimal-html-consumer.

The root-level npm run dev script, which is recommended under the README.md#Development, also seems to use the simple-html-consumer example as its base, so this too breaks w/o the Mapbox API keys.

After that I switched tracks, since it seemed like the examples weren't really intended for development, but for the farmos.github.io only. So I initialized a vanilla Vite project with farmOS-map as an dependency consumed via npm. Here I ran into a number of errors, based largely upon the way that the root package.json points to src/index.js as the main entry point, but there seem to be some bugs if not using Webpack and/or a similar configuration as the root webpack.config.js.

One issue was with clusterStyle import, which frankly I don't fully understand, except it might be looking in the wrong node_modules directory, or something to do with the order of the imports, maybe?

✘ [ERROR] No matching export in "node_modules/@farmos.org/farmos-map/src/styles.css" for import "clusterStyle"

    node_modules/@farmos.org/farmos-map/src/instance/methods/layer.js:18:22:
      18 │ import colorStyles, { clusterStyle } from '../../styles';
         ╵                       ~~~~~~~~~~~~

I circumvented this error simply by removing the offending line in my node_modules/.../layer.js for the time being, but then I ran into a separate runtime error with the way the OpenLayers CSS are being imported:

Failed to resolve import "/home/jamie/Code/farmOS/trace-n-point/node_modules/ol/ol.css" from "node_modules/.vite/deps/@farmos__org_farmos-map.js?v=8efa65a3". Does the file exist?

I suspect that this is avoided with the distributable b/c Webpack bundles that CSS and adds the proper script tag to the example index.html or just injects it into the JS bundle itself. The README has some recommendations for this, but I'm not really sure I understand these instructions. It states:

Naturally, this requires that the farmOS-map.js and farmOS-map.css files are already included in the page as described in the Usage instructions above.

But that seems to infer one is consuming farmOS-map.js and farmOS-map.css from dist/, yet the package.json points to src, so that is where most bundlers and Node projects will look for them.

So I guess I'm left with two main questions:

1. Would it be possible/desirable to add a separate distributable that bundles all the necessary scripts and styles at separate entry points that don't involve attaching the to the global window object, like dist/farmOS-map.js does, but make them more bundler-agnostic and npm-friendly than drawing directly from src/? Something like, say, lib/index.js and lib/index.css?
2. Are the examples intended to be used for the development environment, or is there a better method? Is it expected that one has their own Mapbox API keys? Or could we add an OpenStreetMap example like I suggested above?

[symbioquine]

Thanks for opening this @jgaehring! This stuff should definitely work more smoothly - or at the very least be clear about which use-cases are supported.

npm run dev from the root of the package seems to be working for me out of the box (if you'll pardon the pun) - albeit with two tiny work-arounds;

• export NODE_OPTIONS=--openssl-legacy-provider (See https://stackoverflow.com/questions/69394632/webpack-build-failing-with-err-ossl-evp-unsupported)
• Manually clicking on the layer switcher and choosing "OpenStreetMap" instead of the "MapBox" ones

I agree having the main example default to "MapBox" is a little weird, we probably should change that, but you probably won't notice it much if once you choose "OpenStreetMap" as I described above because farmOS-map should remember that selection.

to experiment a bit with the examples. To start off, I found the README a bit confusing in the way it describes using the install-all script "in each example project", when the install-all script is not included in each example, but at the root of the examples directory itself. It also doesn't mention that the examples depend upon the distributable version of the library, which needs to be built first.

We should definitely improve the language there and add a note about building the top-level package first. (or maybe do it automatically somewhere in the flow that doesn't cause it to get rebuilt unnecessarily too much)

I suspect that this is avoided with the distributable b/c Webpack bundles that CSS and adds the proper script tag to the example index.html or just injects it into the JS bundle itself. The README has some recommendations for this, but I'm not really sure I understand these instructions. It states:

Naturally, this requires that the farmOS-map.js and farmOS-map.css files are already included in the page as described in the Usage instructions above.

But that seems to infer one is consuming farmOS-map.js and farmOS-map.css from dist/, yet the package.json points to src, so that is where most bundlers and Node projects will look for them.

Yeah, that's a bit confusing. That part of the documentation is definitely aimed at folks who are wanting to build against the code in farmOS-map, but externalize the dependency to the dist artifacts. Vite also has a mechanism for handling externals as I recall, so I think it should work similarly there.

That said, the other use-case would (potentially - this might be a bit more into the untested tall grass) be building farmOS-map as a whole into your application. In that scenario, I think your application bundler would handle the mechanism by which the CSS is bundled/loaded.

Some of the discussion in the following issues may be helpful;

• farmOS-map packaging and module exports #113
• 2.x part #3 - Decouple MapInstanceManager from window.farmOS.map #117

[jgaehring]

Ohhh, this is tremendously helpful, just at a glance (need to be stepping away from this for the day unfortunately). I'm happy to submit changes to the README if I can (wouldn't have been so nitpicky if I didn't intend to 🙂) and if I can find some ways to incorporate those suggestions back into the project, and you think they'd be helpful, I'm happy to open some PR's for that, too. Should make for easier integration with more projects. Thanks for the insights!

[jgaehring]

I'm wondering about this possibility again:

1. Would it be possible/desirable to add a separate distributable that bundles all the necessary scripts and styles at separate entry points that don't involve attaching the to the global window object, like dist/farmOS-map.js does, but make them more bundler-agnostic and npm-friendly than drawing directly from src/? Something like, say, lib/index.js and lib/index.css?

Is the dist/ directory only or primarily intended for the https://farmos.github.io/farmOS-map example?

If so, this strikes me as a bit counter-intuitive as a library, where the distributable is inferred to be the library itself, not an example application, but perhaps there are other factors I'm not privy to. The reason I ask is I think it would be ideal to be able to import farmOS-map or parts of farmOS-map using the regular module syntax (ie, import/export), rather than accessing the global window.farmOS variable, but I'm still getting all kinds of errors whenever trying this, like I mentioned above with the clusterStyle function and ol.css. And I've tried it now with both Vite and Webpack to no avail.

I realize that straight-up changing that would be a huge breaking change, so I wonder if in the meantime we could structure things a bit differently, as:

.
├── dist/      # Decide long-term purpose of this; deprecate all else.
├── examples/  # No change.
├── lib/       # For npm package consumers, in the following formats:
│   ├── cjs/   #   - CommonJS (ie, using require() instead of import)
│   ├── esm/   #   - ECMAScript Modules
│   ├── umd/   #   - Universal Module Definition
│   ├── farmOS-map.css # optional
│   └── farmOS-map.js # optional
├── src/       # No change.
└── www/       # Build artifacts for https://farmos.github.io/farmOS-map.

While I think there are arguments for how dist/ could be differently structured long-term, in the meantime we could use lib/ as a stand-in for that while we experiment, and start moving the example page artifacts over to www/, so once there is a consensus on what the ultimate structure and purpose of dist/ should be, it could remain the same.

Looking at the lib/ directory, we could output the same farmOS-map.js and .css files there that are being output currently to the dist/ directory now, though not necessarily if we don't want to keep that naming convention. What's more important is what our package.json's main property points to. That should be one of the entry points in the cjs/, esm/ or umd/ directories, and opinions may differ, but I'd probably suggest going with the ESM entry point (eg, lib/esm/index.js). The only reason to use CommonJS or UMD would be if you expect some downstream consumers might be using require() or some other method of importing and/or bundling that doesn't support ES2015 import statements, but that's unlikely for a package that only makes sense to run in a browser.

The critical thing about main is that this will ease some of the pain when dealing with downstream bundlers etc (whether that's Webpack, rollup or Vite), since it's a standard property in all npm packages and they will all look for when determining the entry point. We probably want to use something like the sideEffects flag in the package.json for the CSS, but that could be worked out, along with chunking and other stuff if we want. But overall this should simplify a lot, both for us and downstream consumers, by deferring to the consumer's package manager (or lack thereof) for all typical usage, and for anything else, well, they can always just point to the entry point of their choosing or muck around with their own bundler's config or what have you.

The other nice thing about this is we can make the whole "clobbering the global window variable" thing optional, if we make createInstance or MapInstanceManager the default export of our lib/esm/index.js, and just allow main.js to be imported separately or wrap it in a function as a named export alongside the default. I can't really seeing much widespread adoption for farmOS-map as a package dependency in standard JS projects if that global is not made optional.

And sorry for making this such a long tome, but I was going to try to open a PR for this then I ran out of time to get a working implementation together and have to put this aside for now. I may also be unable to follow up for the next few weeks on this issue (currently on a collision course w/ a major burnout), so I wanted to just get all my thoughts and rationale out of my head before it was lost to the void. 🌀

[mstenta]

I probably don't know enough to respond to all this thoroughly, and sorry if I say something dumb/obvious, but maybe it's helpful... :-)

Is the dist/ directory only or primarily intended for the https://farmos.github.io/farmOS-map example?

Apart from examples, dist is also used to package the "built" farmOS-map.js files that are included with releases (for example, the v2.2.1-dist.zip file attached to https://github.com/farmOS/farmOS-map/releases/tag/v2.2.1), which is what farmOS itself actually uses to pull in the map code as a simple JavaScript library (without any npm build/bundling step on the farmOS side).

You can see how the dist directory is used in the GitHub Action for building packaged releases, which runs when a tag is pushed to the repo:

farmOS-map/.github/workflows/release.yml

Line 31 in c73bd7c

mkdir ${{ env.RELEASE_VERSION }}-dist && cp dist/farmOS-map* ${{ env.RELEASE_VERSION }}-dist && zip -r ${{ env.RELEASE_VERSION }}-dist.zip ${{ env.RELEASE_VERSION }}-dist

Here is where we look for that file in farmOS: https://github.com/farmOS/farmOS/blob/9587872ea9e42f749ebf123214dd17ed0367fcf1/composer.libraries.json#L13

The reason I ask is I think it would be ideal to be able to import farmOS-map or parts of farmOS-map using the regular module syntax (ie, import/export), rather than accessing the global window.farmOS variable,
we can make the whole "clobbering the global window variable" thing optional

Is it not optional currently? 🤔

I believe #117 made this possible, and window.farmOS is only used when main.js gets included (correct me if I'm wrong @symbioquine!)...

Quoting that PR:

Why? Better support alternative bundling strategies
and scenarios where having mutable state in the global
window.farmOS.map is ugly/limiting.

These changes should allow farmOS-map to also be used
as follows;

npm install @farmos.org/farmos-map@2

import MapInstanceManager from '@farmos.org/farmos-map/MapInstanceManager';

const maps = new MapInstanceManager();

const map = maps.create('my-map');

It sounds like you're experiencing issues with that code specifically @jgaehring?

but I'm still getting all kinds of errors whenever trying this, like I mentioned above with the clusterStyle function and ol.css. And I've tried it now with both Vite and Webpack to no avail.

Maybe we should drill into that specifically? Were there other specific errors?

[symbioquine]

I believe #117 made this possible, and window.farmOS is only used when main.js gets included (correct me if I'm wrong @symbioquine!)...

That's definitely the intention... though I think it may still be necessary to manually include the CSS somehow.

[jgaehring]

import MapInstanceManager from '@farmos.org/farmos-map/MapInstanceManager';

Ohhh, maybe I was trying to import the wrong function/class. I was looking for a way to get the createInstance function, but I could probably make use of MapInstanceManager to the similar ends. I'll have to try that. I may have also been confused between main.js, which attaches the global, and index.js, which I see now does not.

Still, assuming that works, I believe there could be better ways of restructuring the build process (and/or README) to be more in sync with typical JS development workflows and expectations. I know we looked at this a little in #188, but at the time I was too unfamiliar with the updates to farmOS-map's intended usage since 2.x (eg, I don't recall there being a MapInstanceManager in 1.x, or at least, not when I was using it in Field Kit). Working on some features for SurveyStack (it's worth noting, they still essentially use /src/instance/instance.js as a "copy-n-paste module", which was part of the reason for my confusion), I think I've finally gotten to fully grok what's going on. And as it happens, I was just about to come back and share this article from the Rollup wiki, which I think explains the point I was trying to make above much more clearly:

Typically, a library will be accompanied with a package.json file (this is mandatory if you're publishing on npm, for example). That file will often specify a pkg.main property - something like this:

{
  "name": "my-package",
  "version": "0.1.0",
  "main": "dist/my-package.js"
}

That instructs Browserify or Webpack or [insert module bundler here] to include the contents of dist/my-package.js – plus any dependencies it has – in your bundle when you call require('my-package') in your app or library.

But for ES2015-aware tools like Rollup, using the CommonJS (or Universal Module Definition) build isn't ideal, because we can't then take advantage of ES2015 module features. So assuming that you've written your package as ES2015 modules, you can generate an ES2015 module build alongside your CommonJS/UMD build:

{
"name": "my-package",
"version": "0.1.0",
"main": "dist/my-package.umd.js",
"module": "dist/my-package.esm.js"
}
Now we're cooking with gas - my-package continues to work for everyone using legacy module formats, but people using ES2015 module bundlers such as Rollup don't have to compromise. Everyone wins!

I've been doing something similar for farmOS.js (I forgot until now there is also a browser property you can also use), and I just setup the feature I'm working on for SurveyStack with a similar package.json declaration.

... though I think it may still be necessary to manually include the CSS somehow.

This is something I've also encountered, but once again, Rollup has a rollup-plugin-import-css that makes this pretty easy to manage, as I'm doing in my own Rollup config. I'm not sure if there is more rationale for chunking all the JS and CSS as I see being done in the dist/ directory currently, but I think if these steps are taken to allow the consumer's bundler or other tooling of choice interpret the project structure, that should no longer be necessary, and you're left with a single CSS file, along with 2 or 3 JS files for the different module systems, but that's it.

I imagine Webpack has similar tooling like this, but it might be worth considering a migration to Rollup in the future, particularly if and when a Webpack upgrade is required. Rollup is drastically simpler to configure and keep up-to-date, and it has been the de facto standard for library bundling for the last 5 years now or more. It's also nice that it is the bundler underlying Vite (combined with esbuild for faster transpilation, compared to Babel), so it works well to pair Rollup as a library bundler with Vite as an app bundler, since they can share configuration.

As for the README, reflecting on this now, I think it would make sense to include something at the beginning of the "Usage" section that explains the separate workflows for the global instance and the instance manager before jumping straight into the various methods for creating maps and layers. Neither workflow strikes me as particularly intuitive, and that's not meant as any shade, just that I think it merits some explicit instructions to point out that:

1. The dist/farmOS-map.js file attaches a farmOS object to the global window object upon loading, and that you can access its methods and properties directly if you've already included that in a <script> tag, which is the case for farmOS/Drupal.
2. The src/index.js module (or whatever that may be changed to, if we did something like I outlined above) exports a MapInstanceManager class that can be imported and instantiated if one prefers not to use the global, which is often preferable when using it Node-based JavaScript/TypeScript bundlers and similar tooling.

I feel I made a bit of fuss over getting too specific into possible Webpack configs and all in the "Usage" or "Installation" sections, but to my mind that is not quite the same as making it clear what the main interface is for interacting with farmOS-map instances. I still don't that consider part of the "Installation" process per se, but I do think it merits placement at the very top of the "Usage" section.

[mstenta]

Cool! Whatever we can do to make it easier to use farmOS-map in common and best-practice contexts, I'm all for! I don't have a strong opinion on the specifics, as I don't work in that world myself, so I'll leave those decisions to other stakeholders. :-)

Fully agreed on the README.md thoughts too @jgaehring!

though I think it may still be necessary to manually include the CSS somehow.

Ah gotcha. Perhaps we should open a dedicated follow up for that piece specifically? While leaving this open to broader ideas?

I'm not sure if there is more rationale for chunking all the JS and CSS as I see being done in the dist/ directory currently, but I think if these steps are taken to allow the consumer's bundler or other tooling of choice interpret the project structure, that should no longer be necessary, and you're left with a single CSS file, along with 2 or 3 JS files for the different module systems, but that's it.

Here is the PR to give that more context: #116

The tl;dr is: this is mainly for consumers of the pre-built farmOS-map.js library (the primary consumer is farmOS itself). In the farmOS context, we are including the pre-built JS file, and allowing other farmOS module to add their own behaviors (via their own pre-built JS files) alongside it. So there is no way to know what code will be needed on any given page. We don't have the ability to "tree shake" that a Node-based build process has, in other words, because we don't know the code necessary until runtime. #116 added this concept of "chunks" to our build process, which "magically" splits the built farmOS-map.js code into multiple files, and the main farmOS-map.js code knows how to dynamically include chunks as needed, when code that needs them is loaded. That's my layman's understanding anyway. 😅

[jgaehring]

In the farmOS context, we are including the pre-built JS file, and allowing other farmOS module to add their own behaviors (via their own pre-built JS files) alongside it.

Ohhhhhhh, this was the missing piece of the puzzle for me actually. For some reason I thought those chunks were for other JS bundlers, but they're for farmOS itself then? And I assume there's some PHP somewhere that adds the proper script tags to the HTML, based on whatever it finds in there? That makes a lot more sense, I was confused by how they'd all end up being added back to the index.html or imported by a downstream JS consumer with its own bundler; seemed unnecessarily hard for that purpose. I'm sure I still don't grok it all fully, but no need to bother over it if it's working there.

Perhaps we should open a dedicated follow up for that piece specifically? While leaving this open to broader ideas?

Maybe. There may already be a way this is figured in when using the MapInstanceManager, but I haven't had a chance to try it out that way yet. In any event, maybe it's worth a separate issue for restructuring the JS/CSS module exports in an incremental fashion, if there are sound reasons to go down that route.

[mstenta]

And I assume there's some PHP somewhere that adds the proper script tags to the HTML, based on whatever it finds in there?

Yes exactly! Basically PHP code in farmOS core says "add farmOS-map.js" to the page whenever farmOS is rendering a page that includes a map. And other farmOS modules might say "also add my-map-behavior.js to the page too". Then Drupal adds each JS file via a <script> tag in the HTML it generates and sends back in the server response. Then, in the client's browser, both JS files are loaded separately and the code is run within the client context.

The challenge we were running into was: with each new feature/option we added to the farmOS-map library, that farmOS-map.js file was getting bigger and bigger. And that meant it had a lot of code in it that may not be needed on every page. So the work done in #116 added this "chunking" behavior to make that more efficient. Now farmOS-map.js is relatively small, and additional code gets loaded on-the-fly via the chunks as necessary.

So if you look at the map JS that is being loaded on the farmOS dashboard, for instance, it may be different than the map JS loaded in a different context (like a quick form with a simpler map). That's the idea anyway. It's primary purpose was performance, given the unique modular uncertainty of client-side code in farmOS core. :-)

[jgaehring]

import MapInstanceManager from '@farmos.org/farmos-map/MapInstanceManager';

const maps = new MapInstanceManager();

const map = maps.create('my-map');

It sounds like you're experiencing issues with that code specifically @jgaehring?

but I'm still getting all kinds of errors whenever trying this, like I mentioned above with the clusterStyle function and ol.css. And I've tried it now with both Vite and Webpack to no avail.

Maybe we should drill into that specifically? Were there other specific errors?

FYI, I'm getting the same issue with MapInstanceManager. Here's what I tried:

import MapInstanceManager from '@farmos.org/farmos-map';
import geotraceCtrl from 'farmos-map-geotrace';

const units = 'metric';
const mapMgr = new MapInstanceManager();
const instance = mapMgr.create('map', { units });
instance.addLayer('vector', {
  title: 'Drawing',
  group: 'Editable layers',
  color: 'orange',
});

And here once again is that same error:

✘ [ERROR] No matching export in "../../node_modules/@farmos.org/farmos-map/src/styles.css" for import "clusterStyle"

    ../../node_modules/@farmos.org/farmos-map/src/instance/methods/layer.js:17:22:
      17 │ import colorStyles, { clusterStyle } from '../../styles';
         ╵                       ~~~~~~~~~~~~

5:39:33 PM [vite] error while updating dependencies:
Error: Build failed with 1 error:
../../node_modules/@farmos.org/farmos-map/src/instance/methods/layer.js:17:22: ERROR: No matching export in "../../node_modules/@farmos.org/farmos-map/src/styles.css" for import "clusterStyle"
     at failureErrorWithLog (/home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1636:15)
     at /home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1048:25
     at /home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1512:9
     at runMicrotasks (<anonymous>)
     at processTicksAndRejections (node:internal/process/task_queues:96:5)

[mstenta]

Hmm yea that jives with what @symbioquine said above:

though I think it may still be necessary to manually include the CSS somehow.

Sounds like that's a specific bug to be fixed (but maybe can be worked-around in the meantime?)

[jgaehring]

Sounds like that's a specific bug to be fixed (but maybe can be worked-around in the meantime?)

Might be more trouble than it's worth for a workaround, honestly, b/c I don't really know how that would be done at the moment, but a dedicated build process for module systems (both bundlers and native ECMAScript module usage, neither of which seems to work currently) wouldn't be terribly hard.

For reference, based on the Rollup config I'm using currently for the project I mentioned above, I think a pretty straight forward configuration like that could achieve something similar here, while also potentially opening up a pathway for a less complex, more easily maintained build process in the long run.

[jgaehring]

And to clarify, I don't think there's a bug w/ clusterStyles in particular, though there may be a workaround to address it specifically in this case.

The bug, imo, is that so long as the package.json points to src/index.js and CSS and SVG files are being loaded via farmOS-map's specific Webpack config and retinue of plugins, there's no guarantee the whole thing won't break unless the same config is replicated by downstream consumers (something I have not yet figured out how to do, either w/ Webpack or Rollup). It would take a major overhaul of how those non-JS assets are imported, which seems unnecessary, or provide a separate distributable that works for JS modules.

[symbioquine]

I'm just catching up here...

FYI, I'm getting the same issue with MapInstanceManager. Here's what I tried:

import MapInstanceManager from '@farmos.org/farmos-map';
import geotraceCtrl from 'farmos-map-geotrace';

const units = 'metric';
const mapMgr = new MapInstanceManager();
const instance = mapMgr.create('map', { units });
instance.addLayer('vector', {
  title: 'Drawing',
  group: 'Editable layers',
  color: 'orange',
});

And here once again is that same error:

✘ [ERROR] No matching export in "../../node_modules/@farmos.org/farmos-map/src/styles.css" for import "clusterStyle"

    ../../node_modules/@farmos.org/farmos-map/src/instance/methods/layer.js:17:22:
      17 │ import colorStyles, { clusterStyle } from '../../styles';
         ╵                       ~~~~~~~~~~~~

5:39:33 PM [vite] error while updating dependencies:
Error: Build failed with 1 error:
../../node_modules/@farmos.org/farmos-map/src/instance/methods/layer.js:17:22: ERROR: No matching export in "../../node_modules/@farmos.org/farmos-map/src/styles.css" for import "clusterStyle"
     at failureErrorWithLog (/home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1636:15)
     at /home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1048:25
     at /home/jamie/Code/farmOS/geotrace/node_modules/esbuild/lib/main.js:1512:9
     at runMicrotasks (<anonymous>)
     at processTicksAndRejections (node:internal/process/task_queues:96:5)

It seems like that import shouldn't have anything to do with the CSS. Presumably something in your build configuration @jgaehring is causing the reference to ../../styles to resolve to the styles.css file when in fact it is intended to refer to src/styles/index.js.