Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

Proposal: drop support for primitives and geostore and make each parser standalone #268

Closed
jgravois opened this issue Mar 11, 2016 · 18 comments

Comments

@jgravois
Copy link
Collaborator

at this point it might be a good idea to trim down the scope of Terraformer

proposal

we continue to maintain the repositories below, and refactor them to follow the pattern in arcgis-to-geojson-utils

we deprecate/retire:

we gut the repo here so that it contains only the documentation site for the parsers

what say you @JerrySievert @patrickarlt @ajturner anyone/everyone else?

i would love help, but i'm not the kind of guy to delegate my own proposals

@JerrySievert
Copy link
Contributor

i know that the wkt parser and the arcgis parser are both in use.

i am also planning on pulling in the primitives and spatial operations into another platform using terraformer. i feel that if we took those away, it would make terraformer unusable.

@JerrySievert
Copy link
Contributor

i would also support moving terraformer to its own organization (ala koopjs), which would support the additional functionality without completely gutting the module.

@dmfenton
Copy link

I'm using the spatial operations in Winnow. Very useful for me. It would be nice not to have to instantiate primitives to do the spatial operations so I'd support pulling those out. But I'm not sure how valuable Terraformer is without these things.

@jgravois
Copy link
Collaborator Author

@JerrySievert i feel that if we took those away, it would make terraformer unusable.

@dmfenton I'm not sure how valuable Terraformer is without these things.

since we assist with conversion to geojson, isn't it as easy as mixing and matching with Turf.js?

its my impression that the most heavily used parts of Terraformer have always been the conversion tools. my only intention is to whittle down the surface area of the project to what gets the most use and something that would be more manageable for me to oversee personally (working under the assumption that otherwise it would be more or less unmaintained).

i definitely don't want to make any changes that would make the project less useful so i'm happy to receive the feedback.

@JerrySievert
Copy link
Contributor

i feel that if we moved it to its own organization, and opened up the membership a bit, we'd get a little more traction with maintenance.

@dmfenton
Copy link

since we assist with conversion to geojson, isn't it as easy as mixing and matching with Turf.js?

No Turf does not have the same spatial predicates within, intersects, contains. It has some similar functionality where you could probably make those predicates work, but it relies on JSTS, which is slower than Terraform in my testing.

@JerrySievert
Copy link
Contributor

@dmfenton i'm glad to hear that terraformer is faster, we spent a lot of time trying to optimize

@patrickarlt
Copy link
Contributor

I think the best thing to do would be to:

  1. Move Terraformer to it's own org, a la Koop
  2. Split up the main Terraformer Package into
    • terraformer-arcgis - the ArcGIS parser
    • terraformer-wkt - The WKT parser
    • terraformer-spatial - within, intersects, contains, bbox, convex hull, mercator/geographic conversion ect... pretty much everything in the Terraformer.Tools namespace plus within, intersects and contains convenience methods
  3. Deprecate all Terraformer.Primitive stuff
  4. Deprecate Terraformer.GeoStore stuff

@JerrySievert
Copy link
Contributor

i'm ok with (1) and (2), let's discuss (3), and fine with (4).

when do we want to move it all into its own org? i'm excited to get this project active again (and have a team that can help).

@patrickarlt
Copy link
Contributor

Since (3) seems to be the only thing that merits discussion I'll give my stance on it.

From an API design standpoint turning JSON objects (GeoJSON) into JavaScript Objects (Terraformer Primitives) is a huge pain like @dmfenton mentioned. The only real benefit is some convenience when calling utility methods like thing.within(otherThing) as opposed to within(thing, otherThing).

The main cost of Primitives and why I spun the ArcGIS parser out into its own repo is that in browsers using Primitives was simply a lot of extra dependency I didn't use.

If anyone is really interested in keeping primitives I am game but I think the core promise of Terraformer should be to work with GeoJSON not its own base object types. So the new terraformer-arcgis, terraformer-wkt and terraformer-spatial should all work with GeoJSON and if we want we could make a terraformer-primitive that simply curries all their functionality into a nice object.

@jgravois
Copy link
Collaborator Author

jgravois commented Oct 26, 2018

i finally set up a Terraformer GitHub organization and reserved the @terraformer namespace on npm

My ambitions aren't very grand. I'm mostly just laying groundwork to implement what @patrickarlt suggested back in 2016.

to do:

if anyone is interested in chipping in and helping with any of this, the more the merrier!

someday:

it'd be pretty cool to start porting code from https://github.com/Esri/geometry-api-java one method at a time into @terraformer/spatial

I'm on the fence as to whether or not to make the whole thing a lerna monorepo or not.

@JerrySievert
Copy link
Contributor

I can tackle the wkt parser. will look to see if it still makes sense to stick with jison or move to something else.

@mpayson
Copy link

mpayson commented Oct 26, 2018

eager to help on this one @jgravois , let me know when/where

@mpayson
Copy link

mpayson commented Oct 27, 2018

quick questions on approach:

  1. how should we handle potentially shared functions, such as coordinatesContainPoint written in "/spatial" used in "/arcgis", dependency?
  2. appreciate how turf writes benchmarks in addition to tests, should we make that a pattern?
  3. typescript?

@JerrySievert
Copy link
Contributor

this is what I sent John:

Do you plan on moving any of the old code over from the esri org? If so, I can start on modernizing the tooling and code of the wkt module.

Is there specific tooling you’re planning on?

And is there an overall architecture of how you’re thinking everything should fit together?

@jgravois
Copy link
Collaborator Author

jgravois commented Oct 28, 2018

@mpayson

good question! my instinct is that it'd make the project the most approachable if each of the three new libraries are entirely standalone.

perhaps we can duplicate code initially and then publish a 'helper' dependency in the same org and add it to each bundle?

12/28/2018 edit: the only three i count are coordinatesContainPoint() and pointsEqual() and array(s)Intersect(s)Array().

benchmarks

I agree! I don't have any experience including them in CI, but it'd be awesome to incorporate.

@JerrySievert I've never been much of a tooling nerd. I like a lot of what we're using in rest-js, but I don't have my heart set on anything in particular.

my initial feeling is:

  • JS (not TypeScript)
  • Jasmine for Node.js tests, Karma for browser tests (but happy to consider something else)
  • make it a monorepo

Do you plan on moving any of the old code over from the esri org?

definitely. my plan (as of this morning) is to move this repo into the new org, make it the monorepo, and use new issues here for tracking. if it were me by myself I'd probably lean really heavily on the tooling in https://github.com/esri/arcgis-to-geojson-utils and port it as quickly as possible but if you want to explore something new for the wkt parser I'm a-okay with following your lead in the other two.

is all that still clear as mud?

if you can't already tell, any suggestions/recommendations/questions or demands will be warmly received.

@jgravois jgravois reopened this Oct 28, 2018
@jgravois
Copy link
Collaborator Author

today i finally got around to creating a monorepo and porting @terraformer/arcgis.

https://github.com/terraformer-js/terraformer

my plan now is to start tracking to do items in the new repo. once we add wkt, spatial (and common?), 💅 the JSDoc and ship v2.0.0 i'll archive all the Esri repos.

@jgravois jgravois changed the title Proposal: drop support for primitives, spatial analysis functions and geostore Proposal: drop support for primitives and geostore and make each parser standalone Apr 4, 2019
@jgravois
Copy link
Collaborator Author

jgravois commented Apr 16, 2020

i sat on it awhile, but i finally got around to finishing the port and publishing new packages on npm.

@terraformer/spatial
@terraformer/wkt
@terraformer/arcgis

<script type='module'>
  import { calculateBounds } from 'https://unpkg.com/@terraformer/spatial?module';

  calculateBounds({
    type: "Point",
    coordinates: [ 45, 60 ]
  })

  // [45, 60, 45, 60]
  </script>

i got the motivation i needed when someone in the wild cut their own library size in half by swapping in what i'd already finished and shaking the tree. 🌲

given that no one besides me has responded to issues or pull requests in any Terraformer repos in years, i still think that archiving them, directing folks to terraformer-js/terraformer and closing this issue would be the right call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants