Add GeoCoder Service to examples/todo #1347

bajtos · 2018-05-21T14:57:02Z

This pull request improves our Todo example to demonstrate how to use Services in practice. To do so, we leverage US Census Geocoder (API docs) to add location-based reminders to our Todo items.

Based on the outcome of this work, I'll update our Services docs with best practices on testing, see
#1311, and also update Todo tutorial to match the new Todo example implementation. These documentation updates are out of scope of this pull request though!

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

raymondfeng · 2018-05-21T19:14:57Z

Great work. The only concern I have is that it's not reliable to test Google GeoService due to rate limiting unless we allows optional credentials for Google map. Please note that travis can encrypt env vars - see https://docs.travis-ci.com/user/environment-variables/.

bajtos · 2018-05-22T10:49:29Z

The only concern I have is that it's not reliable to test Google GeoService due to rate limiting unless we allows optional credentials for Google map. Please note that travis can encrypt env vars - see https://docs.travis-ci.com/user/environment-variables/.

It's getting even worse. See https://cloud.google.com/maps-platform/user-guide/

Beginning on June 11, 2018, you’ll need to enable billing with a credit card and have a valid API key for all projects.

I'll look for other alternatives providing a free geocoding API.

bajtos · 2018-05-22T11:44:21Z

I spent more than an hour researching different options. Most hosted solutions are paid or have too strictly rate limit (e.g. OpenStreetMap's Nominatim allows only one request per second). The only reasonable and free solution I found is US Census Geocoder (API docs).

I am going to rework the Todo example to use this service instead of Google Maps.

bajtos · 2018-05-22T15:42:26Z

FWIW, IBM Cloud provides geocoder services too - see https://console.bluemix.net/docs/services/Weather/weather_tutorials_samples.html#using_location_services

bajtos · 2018-06-04T09:39:32Z

I think we need to put this on hold until @virkt25 work on datasource support in CLI and bootstrapper are landed - see #1385. Otherwise we will end up with a bunch of conflicts.

bajtos · 2018-06-14T13:29:44Z

Rebased on top of the latest master and prepared for review.

@raymondfeng @strongloop/sq-lb-apex PTAL.

b-admike · 2018-06-14T14:24:40Z

examples/todo/src/datasources/geocoder.datasource.ts

+  ) {
+    dsConfig = Object.assign({}, dsConfig, {
+      // A workaround for the current design flaw where inside our monorepo,
+      //   packages/repository-proxy/node_modules/loopback-datasource-juggler


did you mean packages/repository/node_modules here instead of packages/repository-proxy/node_modules?

Good catch! I meant service-proxy instead of repository-proxy. Will fix.

b-admike · 2018-06-14T14:32:10Z

examples/todo/test/acceptance/application.acceptance.ts

+    expect(result).to.containDeep(todo);
+  });
+
+  it('creates an address-based reminder', async () => {


would adding a test for retrieval of a todo with remindAtGeo and remindAtAddress set add value to our test suite?

I don't think so - we already have a test to verify retrieval of a todo. Unless you see reasons why retrieval of a todo with remindAt* fields may work differently from retrieval of todos without those fields set?

It's to see that we can retrieve them as well as create them with those fields, but I don't think it's needed tbh.

virkt25

Great work 👏 ! Just have a few minor questions. Overall looks great.

virkt25 · 2018-06-15T14:27:37Z

examples/todo/src/controllers/todo.controller.ts

+    if (todo.remindAtAddress) {
+      // TODO(bajtos) handle "address not found"
+      const geo = await this.geoService.geocode(todo.remindAtAddress);
+      todo.remindAtGeo = `${geo[0].y},${geo[0].x}`;


Are geo coordinates typically stored as y, x and not the standard x, y?

This is a great question! For reference, y is latitude and x is longitude.

As far as I can tell from searching around, there is no wide consensus on the right order :(

In StrongLoop, we were always using lat,lng order to be compatible with Google Maps API, since that's the API most people seem to be using and be familiar with these days .

https://stackoverflow.com/q/7309121/69868

EPSG:4326 specifically states that the coordinate order should be latitude, longitude. Many software packages still use longitude, latitude ordering. This situation has wreaked unimaginable havoc on project deadlines and programmer sanity.

The prefered order is by convention latitude, longitude. This was presumably standardized by the International Maritime Organization as reported here. Google also uses this order in its Maps and Earth. I remember this order by thinking of alphabetic order of latitude, longitude.

BUT:

The correct order is longitude, latitude, in virtually all professional GIS applications, as it is in conventional math (ie, f(x ,y, z)). The GeoJSON standard is fairly typical and succinct:

The order of elements must follow x, y, z order
(easting, northing, altitude for coordinates in a
projected coordinate reference system, or longitude,
latitude, altitude for coordinates in a geographic
coordinate reference system).

The same is true of the primary Open Geospatial Consortium standards (WKT and WKB, and extensions like EWKB). Likewise Google may output the order in Lat/Lon to make it more familiar to users who grew up with that custom (ie from navigation standards like IMO, rather than computational ones.) But the KML standard itself is like virtually all other GIS systems:

https://gis.stackexchange.com/q/7379

We are used to seeing latitude and longitude marked on paper maps, tempting us to think of these as planar (Cartesian) coordinates. However, they are not; the paper (planar) map is a projection of spherical coordinates, and spherical coordinates are generally written as radius, inclination, and azimuth (at least in physics.) In fact, the order radius, inclination, azimuth is codified in ISO 31-11. Geographers don't need the radius (or to the extent they do, they use elevation/altitude, which is the deviation from the nominal radius of the earth), so we just have inclination (latitude) and azimuth (longitude). From this perspective, latitude/longitude is perfectly rational.

I added a comment explaining the order and pointing to those stackexchange/stackoverflow pages.

To be honest, I don't really mind which order we use. If you think we should switch to lng,lat, then please let me know and I'll make that change in a follow-up pull request. (You can also send that PR yourself if you like.)

The convention I'm familiar with / expect myself is lat, long but it was the x,y that threw me ... but we don't exactly have control over that as it's coming from an external service. I'm ok with the changes as they currently stand. Thanks for the explanation and links!

virkt25 · 2018-06-15T14:45:11Z

examples/todo/test/acceptance/application.acceptance.ts

-  before(async () => {
-    await app.boot();
+  let cachingProxy: HttpCachingProxy;
+  before(async () => (cachingProxy = await givenCachingProxy()));


Any reason for the async?

Yes, I need to store the result of asynchronous givenCachingProxy in cachingProxy variable.

virkt25 · 2018-06-15T15:07:12Z

examples/todo/test/acceptance/application.acceptance.ts

+    await app.boot();
+
+    /**
+     * Override DataSource to not write to file for testing. Since we aren't


While you're here can you please update this comment -- Override default config for DataSource for testing so we don't write tests to file when using the memory connector. - Rest isn't relevant as it's being handled by Booter. My mistake for not updating this comment.

virkt25 · 2018-06-15T15:15:30Z

examples/todo/test/helpers.ts

+  return config;
+}
+
+export {HttpCachingProxy};


Is this export necessary?

Not strictly necessary, but it simplifies imports in test files (application.acceptance.ts and geocoder.service.integration.ts). With the re-export in place, I can do the following:

import {HttpCachingProxy, givenCachingProxy} from '../../helpers';

virkt25 · 2018-06-15T15:17:09Z

examples/todo/test/helpers.ts

+const GEO_CODER_CONFIG = require('../src/datasources/geocoder.datasource.json');
+
+export function getProxiedGeoCoderConfig(proxy: HttpCachingProxy) {
+  const config = Object.assign({}, GEO_CODER_CONFIG);


Any reason to not do this:

const config = Object.assign({}, GEO_CODER_CONFIG, {options: {proxy: proxy.url, tunnel: false}})

In this case, the return can be in the same line as well.

Your code snippet won't work because Object.assign performs a shallow merge (only top-level properties are merged) andGEO_CODER_CONFIG already contains options:

{ // ... "options": { "headers": { "accept": "application/json", "content-type": "application/json" } }, // ... }

To avoid future confusion, I'll rework this code to use lodash.merge.

virkt25 · 2018-06-15T16:05:15Z

packages/core/src/index.ts

 // loopback dependencies
-export {inject, Context} from '@loopback/context';
-export {Server} from './server';
+export * from '@loopback/context';


Any reason for the switch to *?

Typical applications need more than just inject and Context. When I revert this change, I see the following compilation errors:

examples/todo/src/application.ts(6,28): error TS2305: Module '"packages/core/index"' has no exported member 'Provider'. examples/todo/src/application.ts(6,38): error TS2305: Module '"packages/core/index"' has no exported member 'Constructor'. examples/todo/src/services/geocoder.service.ts(7,17): error TS2305: Module '"packages/core/index"' has no exported member 'Provider'.

Show how to use REST services (US Census Geocoder) in LB4 apps.

bajtos self-assigned this May 21, 2018

bajtos requested review from b-admike, hacksparrow, raymondfeng, shimks and virkt25 May 21, 2018 14:57

This was referenced May 24, 2018

Make services easy to test + docs #1311

Closed

Update dependencies to latest loopbackio/loopback-connector-rest#120

Merged

Feature: a caching HTTP proxy for integration tests #1364

Merged

bajtos mentioned this pull request Jun 6, 2018

[Spike]How to modify datasource configuration in integration/acceptance tests #1396

Closed

bajtos force-pushed the feat/todo-with-location branch from 996f447 to 2186e8f Compare June 14, 2018 13:23

bajtos changed the title ~~WIP: add Geo Service to examples/todo~~ Add GeoCoder Service to examples/todo Jun 14, 2018

bajtos added this to the June Milestone milestone Jun 14, 2018

b-admike approved these changes Jun 14, 2018

View reviewed changes

bajtos force-pushed the feat/todo-with-location branch from 2186e8f to e98646e Compare June 15, 2018 06:27

virkt25 reviewed Jun 15, 2018

View reviewed changes

bajtos added 3 commits June 18, 2018 10:14

fix(service-proxy): add missing dependency on dist-util

a0d758a

fix(http-caching-proxy): add missing dependency on dist-util

c39d379

feat(example-todo): add Geo to examples/todo

7ceb2c4

Show how to use REST services (US Census Geocoder) in LB4 apps.

bajtos force-pushed the feat/todo-with-location branch from e98646e to 7ceb2c4 Compare June 18, 2018 09:14

bajtos merged commit b4a9a9e into master Jun 18, 2018

bajtos deleted the feat/todo-with-location branch June 18, 2018 09:58

This was referenced Jun 19, 2018

Sugar API for registering services + automated registration via boot #1439

Closed

CLI: a command to create a new service #1312

Closed

Add GeoCoder Service to examples/todo #1347

Add GeoCoder Service to examples/todo #1347

Uh oh!

Conversation

bajtos commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

raymondfeng commented May 21, 2018

Uh oh!

bajtos commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented May 22, 2018

Uh oh!

bajtos commented May 22, 2018

Uh oh!

bajtos commented Jun 4, 2018

Uh oh!

bajtos commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bajtos commented May 21, 2018 •

edited

Loading

bajtos commented May 22, 2018 •

edited

Loading

virkt25 Jun 18, 2018 •

edited

Loading