mbloch / mapshaper

Tools for editing Shapefile, GeoJSON, TopoJSON and CSV files
http://mapshaper.org
Other
3.66k stars 528 forks source link

-require local script file vs. npm package #646

Open indus opened 1 week ago

indus commented 1 week ago

I'm struggling with -require (again ;-)

I'm having a bunch of scripts that extend mapshaper functionality that I use from time to time. To make it easier to use them on other systems I wanted to put them in a registry so I would be able to load them with npm. I was surprised that this makes quite a difference.

Some of my findings:

There are probably good reasons for those differences between those two types of "-require" but maybe you could give me a hint what the best-practice is to create a plugin-library that combines multiple commands - that is easy to distribute, load and call...

mbloch commented 1 week ago

Hi!

I should be able to make some changes to support your use cases. Part of the problem may be that mapshaper is using import() instead of require() to load external modules. import() can load node-style modules, but (as far as I understand) doesn't search the same set of paths as require(), which explains why modules installed locally with npm aren't loading. I could try using require() if import() fails.

I'll look into the global context issues you reported.

indus commented 1 week ago

I investigated a bit further and i think the last bullet (no access to context) is just caused because the function is enforced to be a nested property of the alias and not on the global level.

with this script...

module.exports = {
    fnA:function(){
        console.log(Object.getOwnPropertyDescriptors(this))
    },
    nested:{
        fnB:function(){
            console.log(Object.getOwnPropertyDescriptors(this))
        }
    }
}
mapshaper -require "./index.js" -run "{fnA()}" -run "{nested.fnB()}"

... fnA has context in this and nested.fnB does not

At the moment functions in npm modules are always a nested properties. This make the difference?!

indus commented 1 week ago

Just to give you some context. The functions I use most often generate geojson output based on geojson input. And the template I use looks something like this:

const fnName = "myFn"

module.exports = {
    [fnName]: function (arg1, arg2, arg3) {
        const { $, target } = this;

        if ($) {
            const geojson = $.geojson;

            // generate modified geojson

            return geojsonMOD;

        } else if (target) {

            const fnName_ = `[+ ${fnName}]`;

            console.info(`${fnName_} The target gets cloned and renamed - use '-each ${fnName}(...)' to alter it in place`)

            let args = [...arguments].map(JSON.stringify)

            let name = [target.layer_name, `_${fnName}`, ...args]

            return `-filter "true" target=${target.layer_name} + name=${name.join('_')}
            -each this.geojson=${fnName}(${args.join(',')})`;

        }
    }
}

This allows the function to be called with -run myFn(1,2,3) to create a new layer based on the current target as well as with -each this.geojson = myFn(1,2,3) to change the current target in place with the same arguments.

(maybe I should use the io api of -run to generate new layer instead of copying them with -filter "true"🤔 )

indus commented 1 week ago

I've just tested how to make the context thing work. Turns out that If npm modules would also be asigned to global like so the context would be available with this. This would fix this issue as well.

Maybe the functions in the module should be assigned to global by default and only should nested if alias is used