Module/map/backend #524

chrismclarke · 2019-07-16T16:39:18Z

Main Changes:

[feature] - Add bindings for firebase realtime db
[feature] - Add wrapper for local indexedDB (dexie)
[feature] - Add map data retrieval methods
[feature] - Bind user profile edit to map pin updates
[refactor] - replace settings edit page map to use same map and methods as map page, and standalone component
[refactor] - update map store to accept userstore in constructor so that userstore methods can be used within it (not best pattern but required as map pin details saved within user profiles)
[nit] Add default props to map (simplifies logic where optional params not passed)
[nit] Replace map id field with _id (populated by db)
[nit] Minor user model tidying

Todo

add sync methods for pins to realtimedb
add additional fields for cover/hero image in user profile
map pin preview within profile settings edit page
update user lastActive times on login

chrismclarke · 2019-07-16T16:51:57Z

I see the build is failing but due to FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

Will try find a fix for travis and see if it works

chrismclarke · 2019-07-16T16:56:48Z

(fixed)

b-n

Just some stuff around flow of the stores and the such which I think should be looked at, but in general that's a pretty cool integration there. :-D

b-n · 2019-07-17T10:38:32Z

src/components/Avatar/index.tsx

-    const url = await this.injected.userStore.getUserAvatar(userName)
-    this.setState({ avatarUrl: url })
+  getUserAvatar(userName: string) {
+    return this.injected.userStore.getUserAvatar(userName)


This feels like something that should be done by state and @observer instead of waiting on the store to return something. What you are trying to achieve here should be in a lib, not in a store.

If you're wanting to use the store, then Generally speaking, you would have a placeholder, then on componentDidMount you would tell the store to getUserAvatar. The function would then do this async, set something inside it's internal state for on an @observeable, to which you would then bind that into the component using observer. I see that the code is referenced in the store again in a helper function, but that should really not be public, or should be libbed as above.

makes sense. It used to be an async function which is why it was there (had to make a call to the database to get urls), but then I realised all the database method was actually doing was reformatting a string in a pretty obvious way. Will change.

b-n · 2019-07-17T10:39:54Z

src/models/maps.models.tsx

@@ -1,13 +1,14 @@
-export interface IDatabaseMapPin {
-  id: string
+import { IDbDoc } from './common.models'


Nice. I saw this on the other models but didn't understand it, so thought I'd leave this part to you anyway 👍

b-n · 2019-07-17T10:40:40Z

src/pages/Maps/Content/Controls/index.tsx

@@ -51,7 +51,7 @@ class Controls extends React.Component<IProps> {
          position: 'absolute',
          left: '10%',
          borderRadius: '5px',
-          zIndex: 99999,
+          zIndex: 2,


haha, good hack fix is good

b-n · 2019-07-17T10:43:58Z

src/pages/Maps/Content/View/index.tsx

@@ -79,6 +79,15 @@ class MapView extends React.Component<IProps, IState> {
      </Map>
    )
  }
+
+  static defaultProps: Partial<IProps> = {


Oh I get what you mean on slack now. Yes, for all intents and purposes, the click binders are actually not "required". It is required for a functioning application, but it's not required in the grand scheme of things. In theory this(ese) components could be a bit more generic, and could thus be thrown into the components folder. Specifically the map view stuff. Hrrmm, food for thought another day though.

yeah, my main reason was so I could include it in the profile module without having to write a click handler (was feeling lazy), but I think it also does give that option of making even more standalone. But yeah, later days.

src/pages/Settings/content/UserMapPinEdit.tsx

b-n · 2019-07-17T10:50:20Z

src/stores/Maps/maps.store.ts

-    if (this.pinData.length === 0 || this.availablePinFilters.length === 0) {
-      return []
+  @action
+  private processDBMapPins(pins: IDatabaseMapPin[]) {


Nice. This makes a bit more sense since the @computed stuff was borked. Is the @action part required for mobx? i.e. if you don't have the @action, does it not trigger a reflow on after settign this.mapPins? (if it's not needed, just thinking to remove it etc).

As far as I'm aware @action is really just syntactic sugar, and any changes to this.myobservable will get tracked regardless (what the @observable decorator is doing). It's considered good practice to put the decorator on anything that does directly modifiy an observable for a couple reasons, it helps with code readability and also as it adds debugging info if you have something like mobx-devtools installed on your browser to view action logs and history. It also automatically wraps everything in a single transaction, so that if inner methods change the observable it will only emit a notification after all have processed - good for performance if you're doing that a lot but still I think it's better to just avoid modifying observables unnecessarily.

The @computed would be a much better method if a) it worked, and b) we wanted to trigger on changes to multiple fields (e.g. map pins data as well as list of map filters), but for now is fine and if we do decide to hook up to the db for filters and not just use hardcoded we can just add a manual call after that data is received also.

b-n · 2019-07-17T10:52:20Z

src/stores/Maps/maps.store.ts

+    Database.getLargeCollection<IDatabaseMapPin>(this.mapEndpoint).subscribe(
+      data => {
+        console.log('setting pin data', data)
+        this.processDBMapPins(data)


It would be good to throw this function also into retrievePinFilters. The action to set mapPins requires two separate call outs, and thus it could be triggered from either one of them. It makes me think though, that maybe it should just be one @action, with a call to both sub functions, and then on a Promise.all to set the mapPins. I could put together a PR for that if you want to see what I'm talking about?

I get what you're saying, definitely nice to avoid repeatedly processing the same data. Although for now I'm happy to keep the filters hardcoded as I really can't imagine them changing very much, but would be a nice feature if you wanted to implement for v2.

The other option would be to simply queue up the operations, as the database filters could be retrieved with a single call (I think there is a Database.getCollection(collectionName,'once') or similar method which resolves a promise instead of an observable. We could have an initialisation that awaits getting filters and then subscribes to data changes.

The other thing I should add to a future to-do list would probably be to highlight a list of what data has changed when the observable emits pins, so potentially that could just be streamed in or updated where required, which I think would also help here...

b-n · 2019-07-17T10:58:54Z

src/stores/Maps/maps.store.ts

-    if (!this.pinDetailCache.has(id)) {
-      // TODO: get from database
-      this.pinDetailCache.set(id, generatePinDetails(pin))
+  public async getPinDetails(pin: IMapPin): Promise<IMapPinDetail> {


I think this function shouldn't actually return anything since it should be setting this.pinDetail. I would see this a bit more as follows:

public async getPinDetails(pin: IMapPin) { if (!this.pinDetailCache.has(pin._id)) { const pinDetail = await this.getUserProfilePin(pin._id) this.pinDetailCache.set(pin._id, { ...pin, ...pinDetail }) } this.pinDetail = this.pinDetail = this.pinDetailCache.get(pin._id) }

Basically, if cache miss, fill cache, then set value. If no cache miss, set value. I've just done a find on the getPinDetails function and can't see the return being used anywhere, thus the reason 👍

good catch, yeah I had originally put it there whilst testing on the user profile settings page, but if we want the user to be able to click on their map pin there also it should be linked fully to the current implementation instead of passing and calling itself

b-n · 2019-07-17T11:00:05Z

src/pages/Settings/content/UserMapPinEdit.tsx

+  private async loadUserPin() {
+    const userPin = await this.injected.mapsStore.getPin(this.user.userName)
+    this.setState({ userPin })
+  }


Same comment as first part. I would either make a observeable value in the store, or put the function from this into a lib.

yeah, I really hated this pattern. sounds like a good solution, will implement

b-n · 2019-07-17T11:03:36Z

src/stores/index.tsx

  templateStore: new TemplateStore(),
  tagsStore: new TagsStore(),
  platformStore: new PlatformStore(),
  eventStore: new EventStore(),
  discussionsStore: new DiscussionsStore(),
-  mapsStore: new MapsStore(),
+  mapsStore: new MapsStore(userStore),


I see what you're doing here, but I think the flow of those functions needs to be changed a bit. It feels more like the user store would want the map store, but i'm not sure if that makes sense either. i.e. a mapPin is part of a user, where as a user is part of a map doesn't make the most sense.

strongly agreed. I think one of the common workarounds is to have a shared/root store which can allow some sharing across stores, but I think for the current use case your suggestions sound like a better option. Will rethink and get back to you when something's done.

chrismclarke · 2019-08-16T19:04:17Z

Starting to work back on this although getting a bit side-tracked on general db things which would be better in another branch. Will close here for now, do some cherry-picking and re-open once better organised and (hopefully) ready for merge

chrismclarke added 13 commits July 12, 2019 16:13

user pin edit component, move from settings page

234ddfb

add default map props

b277f45

add _lastActive to user modal and remove location

2351682

add user pin retrieval methods, renamie map pin edit page

3d2a9d3

user pin save and retrieval methods

3337a04

rename pin edit page

b177936

update map models

5cc1175

add backend methods to get large map collections

3712070

minor housekeeping

f60315b

remove map 'id' for auto populated '_id'

c05a62d

fix mocks and user pin methods

8e14464

fix map z-index so login appears above

5b2bcfc

fix map computed method not firing correctly

9696006

chrismclarke requested review from b-n and BenGamma July 16, 2019 16:39

chrismclarke mentioned this pull request Jul 16, 2019

Discussion - Optimising map backend #518

Closed

b-n requested changes Jul 17, 2019

View reviewed changes

chrismclarke added 3 commits August 15, 2019 12:44

move IDBEndpoints type to models, rename

dd94429

WiP - firestore -> firebase rtdb sync methods

2059f05

improve backend workflow and readme docs

d83c21c

chrismclarke closed this Aug 16, 2019

chrismclarke mentioned this pull request Aug 28, 2019

Module/map/backend v2 #550

Merged

chrismclarke deleted the module/map/backend branch September 10, 2019 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module/map/backend #524

Module/map/backend #524

chrismclarke commented Jul 16, 2019

chrismclarke commented Jul 16, 2019

chrismclarke commented Jul 16, 2019

b-n left a comment

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

b-n Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

b-n Jul 17, 2019

chrismclarke Jul 17, 2019

chrismclarke commented Aug 16, 2019

Module/map/backend #524

Module/map/backend #524

Conversation

chrismclarke commented Jul 16, 2019

chrismclarke commented Jul 16, 2019

chrismclarke commented Jul 16, 2019

b-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismclarke commented Aug 16, 2019