Es/map server #708

ErikSin · 2022-11-28T23:29:55Z

Map Server Integration

src/renderer/hooks/useMapServerQuery.js

src/renderer/hooks/useMapServerQueryWithImplicitTypings.js

ErikSin · 2022-11-28T23:38:03Z

@gmaclennan I created 2 hooks (technically 1 hook and an API layer) to show the difference in typescript typings. If we do create an API layer, we can control the typing a little easier. And react-query will infer the typings from the api type assertions.

If we don't have an API layer, we will need to assert the type to the hook everytime we use it, which gets a little clunky with tanstack typings.

I know you didn't want to create that extra layer, so I am curious if you think there is a more efficient way of doing this?

gmaclennan · 2022-11-29T16:17:49Z

@ErikSin see my commit 70fad5f for an example of how you might do this.

ErikSin · 2022-11-29T22:00:45Z

@ErikSin see my commit 70fad5f for an example of how you might do this.

Unfortunately not working as expected. With styles/${string} I should be expecting a styleJson. But I am still getting a MapServerStyleInfo

Also getting a weird error, related to typing a function:

Edit:

I see now that the second error is becuase i am trying to type a function. But even if I convert it to a const im still getting errors:

gmaclennan · 2022-11-30T11:24:14Z

Indeed, it seems like function overloads are not really supported in JSDoc, which is a shame. I think the best way around this is a manual d.ts file for the hook. I've pushed an example to a separate branch: 53ac077

Verified locally by running:

❯ ./node_modules/.bin/tsc --noEmit --allowJs --esModuleInterop --jsx react src/renderer/hooks/MapServerExample.js

ErikSin · 2022-12-07T02:51:03Z

package.json

@@ -284,7 +288,7 @@
    "logError": true
  },
  "lint-staged": {
-    "*.{js,ts,tsx}": [
+    "*.{js,tsx}": [


prettier is not playing well with .d.ts files. I will look further into this, but I wanted to push this for review while I did that

for posterity, i believe this is an issue with prettier-standard, see sheerun/prettier-standard#111 (comment). probably not worth trying to fix right now since we're only creating declaration files, but once we do a larger scale migration to TS, we should hopefully have this fixed :)

achou11

Overall seems good! had a couple of comments that may require some action. This is just the integration right? i.e. none of the queries/mutations are being used in the PR

achou11 · 2022-12-15T19:25:07Z

package.json

@@ -34,10 +34,11 @@
    "start": "electron . --disable-http-cache",
    "server": "node index.js --headless",
    "dev": "electron . --debug --disable-http-cache",
-    "postinstall": "npm-run-all -p patch-package extract-presets",
+    "postinstall": "npm-run-all -p patch-package extract-presets electron-rebuild",


Suggested change

"postinstall": "npm-run-all -p patch-package extract-presets electron-rebuild",

"postinstall": "npm-run-all -p patch-package extract-presets rebuild",

needs to reference the name of the npm script command in this case

ahh, that is also why the CI is breaking, I believe!

updated: d165cf3

achou11 · 2022-12-15T21:07:48Z

src/main/index.js

@@ -5,6 +5,7 @@ const { app, dialog, MessageChannelMain, ipcMain } = require('electron')
 const isDev = require('electron-is-dev')
 const contextMenu = require('electron-context-menu')
 const mkdirp = require('mkdirp')
+const createMapServer = require('@mapeo/map-server')


since we're doing TS checking in this file, would update this import to:

Suggested change

const createMapServer = require('@mapeo/map-server')

const createMapServer = require('@mapeo/map-server').default

That way you get the proper TS annotation (see https://github.com/digidem/mapeo-map-server#usage)

Im not sure why, but adding .default just breaks the app.

ah yeah that's on the map server side then, we may have removed that functionality for some reason and didn't update the docs. nevermind then! 😄

achou11 · 2022-12-15T21:08:29Z

src/main/index.js

+  // Fortunately map server does not have any expensive functions so it should
+  // not slow down the main process nor block the render thread if we run it
+  // here from the main process...
+  // @ts-ignore


can remove this if you update the import as suggested in the comment above

achou11 · 2022-12-15T21:26:07Z

src/renderer/hooks/useMapServerQuery.js

+
+export function useMapServerQuery (resourcePath, enabled) {
+  return useQuery({
+    queryKey: [resourcePath],


not super familiar with react query but based on the docs related to query keys, should this be some sort of partitioned array of items based on the path segments of the resourcePath? For example if we're trying to get a specific style /styles/style-id-1, we'd have something like queryKey: ['styles', 'style-id-1'].

Maybe doesn't matter for how we're using it right now, but just curious if that's the more idiomatic approach to the query key

yes, you are right, I will update

updated via: d165cf3

src/renderer/hooks/useMapServerQuery.js

src/renderer/hooks/useMapServerMutation.js

achou11 · 2022-12-15T21:43:31Z

package.json

@@ -284,7 +288,7 @@
    "logError": true
  },
  "lint-staged": {
-    "*.{js,ts,tsx}": [
+    "*.{js,tsx}": [


for posterity, i believe this is an issue with prettier-standard, see sheerun/prettier-standard#111 (comment). probably not worth trying to fix right now since we're only creating declaration files, but once we do a larger scale migration to TS, we should hopefully have this fixed :)

achou11 · 2022-12-15T21:52:49Z

src/renderer/hooks/useMapServerQuery.js

+// @ts-ignore
+const MAP_SERVER_URL = 'http://127.0.0.1:' + window.mapServerPort
+
+export function useMapServerQuery (resourcePath, enabled) {


wondering if the idiomatic approach to using react-query is to create a query for each kind of resource. for example instead of the resourcePath determining what resource we're working with, it could be:

function useGetStylesListQuery(enabled) { return useQuery(['styles'], ...) } function useGetStyleQuery(styleId, enabled) { return useQuery(['styles', styleId], ...) } // etc...

Again, not super familiar with react query best practices but a quick look at the docs seems to lean towards this approach. Guess my reasoning is to make generating the query keys more explicit and easier to understand

Yeah, Gregor and I talked a little about this. We were thinking that we did not want to create too many abstractions, given the simplicity of the API. So we decided to just use the one hook, and have the resource path as the keys. I think when we do more complicated things in the future this would be the way to go.

got it, figured it was discussed but asking for my own education whenever i eventually touch this stuff 😄

achou11 · 2022-12-15T21:54:10Z

src/renderer/hooks/useMapServerMutation.js

+// @ts-ignore
+const MAP_SERVER_URL = 'http://127.0.0.1:' + window.mapServerPort
+
+export function useMapServerMutation (mutationType, resourcePath) {


similar question to https://github.com/digidem/mapeo-desktop/pull/708/files#r1050173134 about creating multiple hooks based on resource

ErikSin commented Nov 28, 2022

View reviewed changes

src/renderer/hooks/useMapServerQuery.js Outdated Show resolved Hide resolved

ErikSin commented Nov 28, 2022

View reviewed changes

src/renderer/hooks/useMapServerQueryWithImplicitTypings.js Outdated Show resolved Hide resolved

ErikSin marked this pull request as ready for review December 7, 2022 02:43

ErikSin commented Dec 7, 2022

View reviewed changes

ErikSin force-pushed the es/map-server branch from 094f4b4 to 69dbd01 Compare December 7, 2022 03:04

achou11 approved these changes Dec 15, 2022

View reviewed changes

achou11 reviewed Dec 15, 2022

View reviewed changes

ErikSin force-pushed the es/map-server branch from 1241f21 to c822271 Compare January 4, 2023 18:49

ErikSin and others added 17 commits January 19, 2023 12:55

chore: added map-server to main thread

21fd811

chore: scaffolded useMapServerQuery

f1932e4

chore: sampled typings

c641fd3

chore: JSDoc overload example

f1e51e6

chore: Added custom hooks

43e171c

chore: update query invalidation

beb245a

chore: update styles endpoint typing to return array

fe432d7

chore: ran electron-rebuild

706d0be

chore: update hooks

1c7ef63

chore: update query invalidation to be more general

7f51587

chore: Pr fixes

2b3f99f

chore: updated timout for git workflow

775eedf

chore: clean up unused varivble

3770f9b

chore: github actions set up vscode dev env

8912498

chore: fix vs code github action

cf866d9

chore: set msvs version

12e6ed0

chore: syntax

3861b54

ErikSin and others added 16 commits January 19, 2023 12:55

chore: syntax

2135b85

chore: set msvs to 2022

a2e0dd3

chore: vscode tooling

a7fdd0b

chore: set msvs to 2017

1268b17

chore: run rebuild github action

a89c075

chore: try global install of node-gyp

186d5be

chore: take out vscode tooling

9cd1ee1

chore: updated github actions

7d64f85

chore: electron-rebuild only run sqlite3

8556e49

chore: debug env

fa76293

chore: revert ci to node 14.18

9976bb6

Hack to fix sodium-native not building

9573a56

chore: update to windows latest

f694e13

chore: remove native deps causing problems in ci

5c749d2

chore: add proper script

1663810

chore: fix rimraf

c32aa42

ErikSin force-pushed the es/map-server branch from d59c2ac to c32aa42 Compare January 19, 2023 20:55

ErikSin merged commit c989c41 into master Jan 19, 2023

ErikSin deleted the es/map-server branch January 19, 2023 22:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Es/map server #708

Es/map server #708

ErikSin commented Nov 28, 2022

ErikSin commented Nov 28, 2022

gmaclennan commented Nov 29, 2022

ErikSin commented Nov 29, 2022 •

edited

Loading

gmaclennan commented Nov 30, 2022

ErikSin Dec 7, 2022

achou11 Dec 15, 2022

achou11 left a comment •

edited

Loading

achou11 Dec 15, 2022

ErikSin Dec 15, 2022

ErikSin Dec 15, 2022

achou11 Dec 15, 2022

ErikSin Dec 15, 2022

achou11 Dec 15, 2022

achou11 Dec 15, 2022

achou11 Dec 15, 2022

ErikSin Dec 15, 2022

ErikSin Dec 15, 2022

achou11 Dec 15, 2022

achou11 Dec 15, 2022 •

edited

Loading

ErikSin Dec 15, 2022

achou11 Dec 15, 2022

achou11 Dec 15, 2022

	"postinstall": "npm-run-all -p patch-package extract-presets electron-rebuild",
	"postinstall": "npm-run-all -p patch-package extract-presets rebuild",

	const createMapServer = require('@mapeo/map-server')
	const createMapServer = require('@mapeo/map-server').default

Es/map server #708

Es/map server #708

Conversation

ErikSin commented Nov 28, 2022

Map Server Integration

ErikSin commented Nov 28, 2022

gmaclennan commented Nov 29, 2022

ErikSin commented Nov 29, 2022 • edited Loading

Edit:

gmaclennan commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 left a comment • edited Loading

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

achou11 Dec 15, 2022 • edited Loading

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

ErikSin commented Nov 29, 2022 •

edited

Loading

achou11 left a comment •

edited

Loading

achou11 Dec 15, 2022 •

edited

Loading