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

added geolocation watch #24

Merged
merged 5 commits into from
Jun 10, 2018
Merged

added geolocation watch #24

merged 5 commits into from
Jun 10, 2018

Conversation

marcosfede
Copy link
Member

active geolocation tracking

@marcosfede marcosfede added the WIP Work In Progress (do not merge yet) label Jun 4, 2018
@marcosfede marcosfede requested a review from jperelli June 4, 2018 03:11
Copy link
Member

@jperelli jperelli left a comment

Choose a reason for hiding this comment

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

copado, nada mas creo que hay un ; de mas

src/utils.ts Outdated
onPermissionChanged: (status: string) => void,
) => {
// Check for Geolocation API permissions
;(navigator as any).permissions
Copy link
Member

Choose a reason for hiding this comment

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

ese ;

Copy link
Member Author

Choose a reason for hiding this comment

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

sep, lo pone el formatter solo..

@marcosfede
Copy link
Member Author

Ahi quedo el flujo de geolocalizacion andando.
Basicamente cree una clase a la cual te podes suscribir 1 vez mediante promise, o N veces mediante un callback, y notifica a todos los observers cada vez que hay un update
Tambien tiene un cache de la ultima posicion y devuelve eso por defecto cuando se requiere una localizacion 1-off
Que cambiarias @jperelli ?

this.start()
}

// resolve from cache if available
Copy link
Member

Choose a reason for hiding this comment

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

joyaaa

src/utils.ts Outdated
}
public addListener(observer: Observer) {
this.takeManyObservers.push(observer)
}
Copy link
Member

Choose a reason for hiding this comment

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

hace falta un removeListener?

Copy link
Member Author

Choose a reason for hiding this comment

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

podria ser, no se donde lo usariamos pero ahi se lo agrego


class GeolocationObservable {
public static getInstance() {
if (!GeolocationObservable.instance) {
Copy link
Member

Choose a reason for hiding this comment

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

singleton ftw!

}

geolocate(){
this.$store.dispatch('geolocate')
this.$store.dispatch('geolocate').then(position => {
Copy link
Member

Choose a reason for hiding this comment

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

aca en vez de usar el .then, directamente se podrian tomar updatingGeolocation, center y zoom de getters del store, y que esas variables se seteen en el store en la action geolocate. O sea, mover el contenido de este then al action

Copy link
Member Author

Choose a reason for hiding this comment

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

el tema es mas que nada que esta funcion corresponde a la logica del boton que centra el mapa en tu geolocalizacion. Necesitas tener una continuidad con la accion de hacer click en el boton, porque no queres centrar el mapa ni zoomear cada vez que hay un update en tu ubicacion, sino solo despues de haber presionado el boton, por eso el .then me parece que va bien en este caso. Otro tema es que center es una variable de estado que solo compete a este componente, se actualiza todo el tiempo al draggear y no se maneja en el store, por eso actualizarla aca y no en una accion en el store, basicamente es estado interno de un componente.
El zoom tampoco es una variable de "estado" sino que es el zoom deseado al presionar ese boton, es una constante que solo compete a este componente.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

},
actions: {
geolocate({ commit, dispatch }) {
return geolocationObservable.takeFirst().catch(e => {
Copy link
Member

Choose a reason for hiding this comment

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

ese catch me parece que nunca se va a llamar.
y creo que es mas claro no retornar nada, y aca mismo hacer un .then que setee center, zoom y updatingGeolocation en el store y eso directamente se refleje en la vista por medio de redux.

retornar una promise en un action me huele a redux anti-pattern, pero no encuentro info al respecto, no termino de cerrar la idea. No se bien, es lo que te decia el otro dia, esto es todo lo que encontre al respecto https://twitter.com/dan_abramov/status/568029790646755328?lang=en es dan abramov, pero falta justificacion, supongo. Encima en este comment parece contradecirse reduxjs/redux#974 (comment) ???

Capaz lo que me hace ruido es esta imagen https://medium.com/@aurelie.lebec/redux-and-react-native-simple-login-example-flow-c4874cf91dde que el flujo de info va para un solo lado, pero agregando un return a un action, hace que haya un flujo hacia atras, del action al container.

Bueno aca encontre una mejor por el creador de vue vuejs/vuex#46 y se contradice con la documentacion oficial de vuex https://vuex.vuejs.org/guide/actions.html#composing-actions

Copy link
Member Author

@marcosfede marcosfede Jun 10, 2018

Choose a reason for hiding this comment

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

Re entiendo lo que decis y veo como va medio en contra del flujo sugerido, los componentes deberian ser dumb, y hacer un "notify and forget" pero creo que no me termina de cerrar la alternativa en este caso particular de feedback de success o fail de la accion asincrona.

La alternativa que me imagino es tener una variable que indique que esta geolocalizando, onda loading, y checkear cada vez que te llega un update a ver si pasa de loading true a false, y en ese caso tambien checkear si la accion fue successful o no, con alguna variable de error en el state. Todo eso ademas lo tenes que crear en un cacho del store separado para este componente, porque no queres que otra parte de la ui que use esta accion de geolocalizacion te afecte el estado de este componente, por eso me pregunto si al fin y al cabo no estoy replicando todo lo que hace la promise por si misma

En este caso igualmente el action de geolocate esta medio al pedo, lo unico que hace es llamar al observable de utils, lo que se puede hacer es llamarlo directamente desde el componente sin pasar por el store, que te parece? Tambien esta el tema de que center es una variable interna que se actualiza banda cuando drageas el mapa, meterla en el store es medio bardo.

Creo que me hace falta leer un poco mas para llegar a una conclusion con esto tb

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Si, hay algo que no cierra del todo... pero bueno, asi como esta funciona joya, por ahi seria bueno seguir con otra cosa y de ultima despues volver a hacer refactor si tenemos alguna revelacion

Copy link
Member Author

Choose a reason for hiding this comment

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

dale!

return geolocationObservable.takeFirst().catch(e => {
dispatch('message', 'No se pudo acceder a la geolocalizacion')
console.error(e)
return Promise.reject(e)
Copy link
Member Author

Choose a reason for hiding this comment

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

Ahora la promise puede ser rejected si falla la geolocalizacion. Que opinas de este catch chaining? Quizas esta action no haria falta y se podria usar directamente el geolocationObservable, pero capaz esta bueno tener este "middleware" que hace dispatch del mensaje de error y pasar el catch hacia el siguiente handler para manejar el error segun le convenga al que llame a esta action

Copy link
Member

@jperelli jperelli Jun 10, 2018

Choose a reason for hiding this comment

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

Si, me parece que esta mejor asi, capaz en este caso no haria falta el console.error(), porque ya estas volviendo a hacer reject?

La diferencia tambien es que quien llame a geolocation tiene que si o si hacer un .catch(), si no se va a romper la app si no recibe la geoloc, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

si no lo cacheas te tira un error en consola, Uncaught (in promise) PositionError {code: 1, message: "User denied Geolocation"} pero muestra el mensaje de error por el middleware y lo demas sigue andando

if (!this.started) {
this.watchId = navigator.geolocation.watchPosition(
position => this.update(position.coords),
err => this.reject(err),
Copy link
Member Author

Choose a reason for hiding this comment

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

si falla la geoloc, por ejemplo porque el permission esta en "blocked" , ahora se rejectean todas las promises que estaban esperando el siguiente tick

this.listenerId++
return id
}
public removeListener(id: number) {
Copy link
Member Author

Choose a reason for hiding this comment

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

agregue el removeListener por si se necesita en algun momento

})
})
}
private reject(error: PositionError) {
Copy link
Member Author

Choose a reason for hiding this comment

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

agregue el reject de las promises, en caso que falle la geoloc

@marcosfede
Copy link
Member Author

mergeo

@marcosfede marcosfede merged commit 42a6aab into master Jun 10, 2018
@marcosfede marcosfede deleted the geolocation-tracking branch June 10, 2018 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work In Progress (do not merge yet)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants