Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support GeoPoint used in loopback 3 #7203

Closed
wants to merge 1 commit into from

Conversation

frbuceta
Copy link
Contributor

@frbuceta frbuceta commented Mar 11, 2021

This is a temporary solution, you have to look for a better option

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • 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

👉 Check out how to submit a PR 👈

@frbuceta frbuceta self-assigned this Mar 11, 2021
@bajtos
Copy link
Member

bajtos commented Mar 12, 2021

Thank you @frbuceta for the pull request! ❤️

I'd like to share few pointers and some information for better context.

In the past, we have been discussing GeoPoint support in LB4 here: #1981, you may find useful information and additional pointers there.

We also have a package loopback-datatype-geopoint which adds helpers like distanceBetween and distanceTo. IMO we should not be depending on that package from LB3 days, but we may want to introduce a similar new package (@loopback/geopoint?) by migrating loopback-datatype-geopoint to TypeScript and this repository.

How Geopoint works in LoopBack 3

The REST API layer supports multiple ways how to represent GeoPoint values:

  • An object {lat, lng} as you are proposing, e.g. ?arg[lat]=2.5&arg[lng]=3
  • An array [lat, lng] - e.g. ?arg[0]=2.5&arg[1]=3
  • Google Location API format lat,lng, e.g. `arg=2.5,3

See here: https://github.com/strongloop/strong-remoting/blob/baa3a5baaae0a1a883dfbe7eddc26982ca071959/test/rest-coercion/urlencoded-geopoint.suite.js#L24-L35

Juggler (data access layer) supports the same three representations:

[lat, lng] or object or a "lat, lng" string

See here: https://github.com/strongloop/loopback-datasource-juggler/blob/cc5557bd1126058e6ee6df9baa7ecb0f51590a86/lib/geo.js#L169-L195

BTW GeoPoint is exported by loopback-datasource-juggler. It may be possible to re-export it from @loopback/repository too, although I am not sure if that's desired. For longer term, I think we should provide GeoPoint type that's decoupled from @loopback/repository and can be used independently e.g. with @loopback/rest only or perhaps with Mongoose.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, your pull request is improving only the part building JSON/OpenAPI schema to understand Juggler properties defined with GeoPoint type. That's a great first step 👍

It makes me wonder though, how would you create such model? How would be the geopoint property described at TypeScript level (what type would it have)? Would you mind to add an integration-level test to show this in practice? See packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts for examples of tests building JSON schema from a full model class created using @model and @property decorators.

@frbuceta frbuceta marked this pull request as draft March 30, 2021 07:39
@stale
Copy link

stale bot commented Jul 14, 2021

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Jul 28, 2021

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

@stale stale bot closed this Jul 28, 2021
@frbuceta frbuceta deleted the feat/support-geopoint-connectors branch August 1, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants