new Node.js demo for batch geocoding a .csv #105

mpayson · 2018-01-24T23:24:56Z

Hey team, first take on a batch geocode sample for Node, would love feedback on:

Is best batch geocode sample a Node script?
General logic/structure/style/formatting (my first PR!)
~~Excel thinks the output .csv is corrupt, not sure why or how to fix?~~ might be local problem

coveralls · 2018-01-25T01:35:49Z

Coverage remained the same at 100.0% when pulling 5b10616 on mpayson:mp-batch-geocode into 7d9a593 on Esri:master.

jgravois · 2018-01-26T17:15:59Z

very, very cool @mpayson!

i've got other fires blazing now, but i'll find time to review and help you get this landed soon.

jgravois

this looks great! we can merge after you have a chance to address my feedback.

jgravois · 2018-02-07T20:30:09Z

demos/batch-geocoder/README.md

@@ -0,0 +1,7 @@
+# Running this demo
+
+1. `$ git clone https://github.com/mpayson/arcgis-rest-js.git`


all the other demo instructions assume the repo has already been cloned.

nothing wrong with you deviating, but if you do, you should point folks at Esri/arcgis-rest-js 😄.

jgravois · 2018-02-07T20:31:24Z

demos/batch-geocoder/README.md

+
+1. `$ git clone https://github.com/mpayson/arcgis-rest-js.git`
+2. `$ cd arcgis-rest-js/demos/batch-geocoder/`
+3. `$ npm install`


since this is a monorepo, we shouldn't direct them to npm install inside individual packages.

instead they should run npm run bootstrap in the root of the repo to appropriately symlink the needed dependencies.

jgravois · 2018-02-07T20:32:10Z

demos/batch-geocoder/batch-geocode.js

+      resultMap = mapResults(res);
+      output = data.map((row, i) => {
+        id = row[config.fieldmap.OBJECTID] || i;
+        return {...row, ...resultMap[id]};


since you're using spread operators and other fancy new JS stuff, you need to make a point in the doc of explaining that Node.js 8.x or higher is required.

✅ added dependency to readme.

I think spread etc only requires >6.x? Can also update to support older versions?

jgravois · 2018-02-07T20:37:29Z

demos/batch-geocoder/config-template.js

+  un: "<USERNAME>",
+  pw: "<PASSWORD>",
+  csv: "<PATH/TO/CSV>",
+  fieldmap: "<CSV TO REQUEST FIELD MAP OR ADDRESS FIELD>"


i'd recommend:

encasing un and your other properties as strings

just hardcoding the .csv path to use your example file.

either doing the same with the fieldmap or at least including it in copy/pasteable form in a code comment in the same file.

{ "fieldmap": { "postal": "ZIPCODE" } }

in general, the fieldmap was the biggest point of confusion when i ran your app. it'd make this sample substantially more self explanatory if you talked through what it is and what it is doing.

jgravois · 2018-02-07T20:38:08Z

demos/batch-geocoder/package.json

@@ -0,0 +1,36 @@
+{
+  "name": "batch-geocoder",
+  "version": "0.0.1",


use the same version number as everything else in the repo. 1.0.2.

jgravois · 2018-02-07T20:38:26Z

demos/batch-geocoder/package.json

+    "arcgis-rest-js"
+  ],
+  "author": "Max Payson (mpayson)",
+  "license": "ISC",


"Apache-2.0"

https://github.com/Esri/contributing#copyright-and-licensing

jgravois · 2018-02-07T20:38:59Z

demos/batch-geocoder/package.json

+    "@esri/arcgis-rest-request": "^1.0.2",
+    "isomorphic-fetch": "^2.2.1",
+    "isomorphic-form-data": "^1.0.0",
+    "papaparse": "^4.3.6"


you should include "private": true to ensure this doesn't accidentally get published to npm.

https://docs.npmjs.com/files/package.json#license

jgravois · 2018-02-07T20:39:53Z

demos/batch-geocoder/package.json

+{
+  "name": "batch-geocoder",
+  "version": "0.0.1",
+  "description": "arcgis-res-js batch geocode sample",


arcgis-res-js > arcgis-rest-js (with a 't')

jgravois · 2018-02-09T19:04:05Z

at some point, it'd be worth considering using something like Sanitize.js or even a simple regex to remove <script> tags from within values inside the .csv (in the unlikely event of an XSS).

jgravois · 2018-02-15T00:24:12Z

are you going to have a chance in the next two weeks to take one more pass at this? i'd like to show it off at DevSummit so i can get it landed if push comes to shove.

mpayson · 2018-02-15T01:41:15Z

@jgravois , yes, I'll have it done by end of week!

Side note, should we include it as part of Node CLI example #112? Then doc in a readme separate files for people looking at batch-geocoding/search/export samples? @dbouwman

mpayson · 2018-02-15T20:25:35Z

jgravois

object spread properties > 8.6.0

other than that, the PR looks great.

jgravois · 2018-02-16T05:29:19Z

should we include it as part of Node CLI example #112?

nah, lets keep them separate. i'd like to see our collection of demos grow to include lots of different browser samples and lots of Node.js samples too.

jgravois · 2018-02-22T21:45:48Z

pushed one more commit to rename the directory batch-geocoder-node and append the context to a few others and rebased.

thx again @mpayson for contributing this sweet demo!

…v in node AFFECTS PACKAGES: batch-geocoder @esri/arcgis-rest-geocoder-vanilla @esri/arcgis-rest-demo-vanilla ISSUES CLOSED: Esri#97

mpayson mentioned this pull request Jan 24, 2018

create example which demonstrates how to batch geocode a .csv #97

Closed

Esri deleted a comment from coveralls Jan 26, 2018

jgravois self-assigned this Feb 1, 2018

jgravois suggested changes Feb 7, 2018

View reviewed changes

jgravois changed the title ~~Mp batch geocode~~ new Node.js demo for batch geocoding a .csv Feb 15, 2018

jgravois reviewed Feb 16, 2018

View reviewed changes

jgravois approved these changes Feb 22, 2018

View reviewed changes

docs(batch geocoder): add new demo to batch geocode addresses from cs…

152c9d8

…v in node AFFECTS PACKAGES: batch-geocoder @esri/arcgis-rest-geocoder-vanilla @esri/arcgis-rest-demo-vanilla ISSUES CLOSED: Esri#97

jgravois force-pushed the mp-batch-geocode branch from 5b10616 to 152c9d8 Compare February 22, 2018 21:53

jgravois merged commit 8a596be into Esri:master Feb 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new Node.js demo for batch geocoding a .csv #105

new Node.js demo for batch geocoding a .csv #105

mpayson commented Jan 24, 2018 •

edited

Loading

coveralls commented Jan 25, 2018 •

edited

Loading

jgravois commented Jan 26, 2018

jgravois left a comment

jgravois Feb 7, 2018 •

edited

Loading

jgravois Feb 7, 2018

jgravois Feb 7, 2018

mpayson Feb 15, 2018

jgravois Feb 7, 2018

jgravois Feb 7, 2018

jgravois Feb 7, 2018 •

edited

Loading

jgravois Feb 7, 2018 •

edited

Loading

mpayson Feb 15, 2018

jgravois Feb 7, 2018

jgravois commented Feb 9, 2018

jgravois commented Feb 15, 2018

mpayson commented Feb 15, 2018 •

edited

Loading

mpayson commented Feb 15, 2018 •

edited

Loading

jgravois left a comment

jgravois commented Feb 16, 2018

jgravois commented Feb 22, 2018 •

edited

Loading

		@@ -0,0 +1,7 @@
		# Running this demo

		1. `$ git clone https://github.com/mpayson/arcgis-rest-js.git`

new Node.js demo for batch geocoding a .csv #105

new Node.js demo for batch geocoding a .csv #105

Conversation

mpayson commented Jan 24, 2018 • edited Loading

coveralls commented Jan 25, 2018 • edited Loading

jgravois commented Jan 26, 2018

jgravois left a comment

Choose a reason for hiding this comment

jgravois Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

jgravois Feb 7, 2018

Choose a reason for hiding this comment

jgravois Feb 7, 2018

Choose a reason for hiding this comment

mpayson Feb 15, 2018

Choose a reason for hiding this comment

jgravois Feb 7, 2018

Choose a reason for hiding this comment

jgravois Feb 7, 2018

Choose a reason for hiding this comment

jgravois Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

jgravois Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

mpayson Feb 15, 2018

Choose a reason for hiding this comment

jgravois Feb 7, 2018

Choose a reason for hiding this comment

jgravois commented Feb 9, 2018

jgravois commented Feb 15, 2018

mpayson commented Feb 15, 2018 • edited Loading

mpayson commented Feb 15, 2018 • edited Loading

jgravois left a comment

Choose a reason for hiding this comment

jgravois commented Feb 16, 2018

jgravois commented Feb 22, 2018 • edited Loading

mpayson commented Jan 24, 2018 •

edited

Loading

coveralls commented Jan 25, 2018 •

edited

Loading

jgravois Feb 7, 2018 •

edited

Loading

jgravois Feb 7, 2018 •

edited

Loading

jgravois Feb 7, 2018 •

edited

Loading

mpayson commented Feb 15, 2018 •

edited

Loading

mpayson commented Feb 15, 2018 •

edited

Loading

jgravois commented Feb 22, 2018 •

edited

Loading