feat: support serving Style Map Package maps #881

achou11 · 2024-10-01T23:17:02Z

Closes #827

Still blocked on the following:

~~Need to add a couple of more tests I think.~~ DONE
~~I didn't upload the fixture that I was using for the tests since I wasn't sure if we had a particular approach for it just yet. I've been using an SMP generated from the command listed here~~ Added a fixture as described
Types are not published with styled-map-package, as that's blocked on fix: include type definitions when publishing styled-map-package#21

Implementation notes:

I made the decision to avoid using the fastify plugin implementation that comes with styled-map-package due to the following reasons:
- it doesn't account for registering it using a prefix. Part of me wonders if i'm holding Fastify wrong and maybe my previous setup for the other plugins is somewhat flawed, but at least when trying to use the plugin in the same way and do similar tests, things started to misbehave when using prefixing.
- it required me to create an extra Reader instance because it doesn't expose the internal one that it creates at all. not the worst thing but figured that given the other issue, I might as well address this issue by re-working the plugin.
open to input about the name of the decorator and the plugin that this PR introduces. I currently call it comapeoSmp and comapeo-styled-map-package, respectfully.
not entirely too happy about the plugin implementation i have right now, which is kind of related to my thinking that maybe i'm holding fastify wrong a bit. open to feedback on cleaner approaches.

awana-lockfile-bot · 2024-10-02T16:25:39Z

`package-lock.json` changes

Summary

Status	Count
	74
	9

Click to toggle table visibility

Name	Previous	Current
`@fastify/static`	7.0.3	7.0.4
`@mapbox/jsonlint-lines-primitives`	-	2.0.2
`@mapbox/sphericalmercator`	-	1.2.0
`@mapbox/unitbezier`	-	0.0.1
`@maplibre/maplibre-gl-style-spec`	-	20.3.1
`@placemarkio/check-geojson`	-	0.1.12
`@turf/bbox`	-	7.1.0
`@turf/helpers`	-	7.1.0
`@turf/meta`	-	7.1.0
`@types/geojson`	-	7946.0.14
`ansi-diff`	-	1.2.0
`ansi-split`	-	1.0.1
`archiver-utils`	-	5.0.2
`archiver`	-	7.0.1
`arr-union`	-	3.1.0
`assign-symbols`	-	1.0.0
`async`	-	3.2.6
`buffer-peek-stream`	-	1.1.0
`bundle-name`	-	4.1.0
`bytewise-core`	-	1.2.3
`bytewise`	-	1.1.0
`cli-spinners`	-	2.9.2
`clone`	-	1.0.4
`compress-commons`	-	6.0.2
`core-util-is`	-	1.0.3
`crc-32`	-	1.2.2
`crc32-stream`	-	6.0.0
`default-browser-id`	-	5.0.0
`default-browser`	-	5.2.1
`defaults`	-	1.0.4
`define-lazy-prop`	-	3.0.0
`extend-shallow`	-	2.0.1
`fastify`	4.26.2	4.28.1
`filter-obj`	-	6.1.0
`get-east-asian-width`	-	1.2.0
`get-value`	-	2.0.6
`into-stream`	-	8.0.1
`is-docker`	-	3.0.0
`is-extendable`	-	0.1.1
`is-inside-container`	-	1.0.0
`is-interactive`	-	2.0.0
`is-plain-object`	-	2.0.4
`is-unicode-supported`	-	2.1.0
`is-wsl`	-	3.1.0
`isobject`	-	3.0.1
`json-stringify-pretty-compact`	-	4.0.0
`ky`	-	1.7.2
`lazystream`	-	1.0.1
`log-symbols`	-	7.0.0
`mimic-function`	-	5.0.1
`minimist`	1.2.7	1.2.8
`normalize-path`	-	3.0.0
`open`	-	10.1.0
`ora`	-	8.1.0
`package-json-from-dist`	-	1.0.1
`parse-ms`	-	4.0.0
`pino-abstract-transport`	1.1.0	1.2.0
`pino-std-serializers`	6.2.2	7.0.0
`pino`	8.20.0	9.4.0
`pretty-bytes`	-	6.1.1
`pretty-ms`	-	9.1.0
`process-nextick-args`	-	2.0.1
`quickselect`	-	2.0.0
`readdir-glob`	-	1.1.3
`run-applescript`	-	7.0.0
`rw`	-	1.3.3
`safe-stable-stringify`	2.4.3	2.5.0
`set-value`	-	2.0.1
`sonic-boom`	3.8.1	4.1.0
`sort-asc`	-	0.2.0
`sort-desc`	-	0.2.0
`sort-object`	-	3.0.3
`split-string`	-	3.1.0
`stdin-discarder`	-	0.2.2
`styled-map-package`	-	1.0.1
`thread-stream`	2.4.1	3.1.0
`tinyqueue`	-	3.0.0
`typewise-core`	-	1.2.0
`typewise`	-	1.0.3
`union-value`	-	1.0.1
`wcwidth`	-	1.0.1
`yoctocolors`	-	2.1.1
`zip-stream`	-	6.0.1

awana-lockfile-bot · 2024-10-02T18:40:12Z

`package-lock.json` changes

Summary

Status	Count
	74
	9

Click to toggle table visibility

Name	Previous	Current
`@fastify/static`	7.0.3	7.0.4
`@mapbox/jsonlint-lines-primitives`	-	2.0.2
`@mapbox/sphericalmercator`	-	1.2.0
`@mapbox/unitbezier`	-	0.0.1
`@maplibre/maplibre-gl-style-spec`	-	20.3.1
`@placemarkio/check-geojson`	-	0.1.12
`@turf/bbox`	-	7.1.0
`@turf/helpers`	-	7.1.0
`@turf/meta`	-	7.1.0
`@types/geojson`	-	7946.0.14
`ansi-diff`	-	1.2.0
`ansi-split`	-	1.0.1
`archiver-utils`	-	5.0.2
`archiver`	-	7.0.1
`arr-union`	-	3.1.0
`assign-symbols`	-	1.0.0
`async`	-	3.2.6
`buffer-peek-stream`	-	1.1.0
`bundle-name`	-	4.1.0
`bytewise-core`	-	1.2.3
`bytewise`	-	1.1.0
`cli-spinners`	-	2.9.2
`clone`	-	1.0.4
`compress-commons`	-	6.0.2
`core-util-is`	-	1.0.3
`crc-32`	-	1.2.2
`crc32-stream`	-	6.0.0
`default-browser-id`	-	5.0.0
`default-browser`	-	5.2.1
`defaults`	-	1.0.4
`define-lazy-prop`	-	3.0.0
`extend-shallow`	-	2.0.1
`fastify`	4.26.2	4.28.1
`filter-obj`	-	6.1.0
`get-east-asian-width`	-	1.2.0
`get-value`	-	2.0.6
`into-stream`	-	8.0.1
`is-docker`	-	3.0.0
`is-extendable`	-	0.1.1
`is-inside-container`	-	1.0.0
`is-interactive`	-	2.0.0
`is-plain-object`	-	2.0.4
`is-unicode-supported`	-	2.1.0
`is-wsl`	-	3.1.0
`isobject`	-	3.0.1
`json-stringify-pretty-compact`	-	4.0.0
`ky`	-	1.7.2
`lazystream`	-	1.0.1
`log-symbols`	-	7.0.0
`mimic-function`	-	5.0.1
`minimist`	1.2.7	1.2.8
`normalize-path`	-	3.0.0
`open`	-	10.1.0
`ora`	-	8.1.0
`package-json-from-dist`	-	1.0.1
`parse-ms`	-	4.0.0
`pino-abstract-transport`	1.1.0	1.2.0
`pino-std-serializers`	6.2.2	7.0.0
`pino`	8.20.0	9.4.0
`pretty-bytes`	-	6.1.1
`pretty-ms`	-	9.1.0
`process-nextick-args`	-	2.0.1
`quickselect`	-	2.0.0
`readdir-glob`	-	1.1.3
`run-applescript`	-	7.0.0
`rw`	-	1.3.3
`safe-stable-stringify`	2.4.3	2.5.0
`set-value`	-	2.0.1
`sonic-boom`	3.8.1	4.1.0
`sort-asc`	-	0.2.0
`sort-desc`	-	0.2.0
`sort-object`	-	3.0.3
`split-string`	-	3.1.0
`stdin-discarder`	-	0.2.2
`styled-map-package`	-	1.0.1
`thread-stream`	2.4.1	3.1.0
`tinyqueue`	-	3.0.0
`typewise-core`	-	1.2.0
`typewise`	-	1.0.3
`union-value`	-	1.0.1
`wcwidth`	-	1.0.1
`yoctocolors`	-	2.1.1
`zip-stream`	-	6.0.1

achou11 · 2024-10-02T18:46:55Z

src/fastify-plugins/maps/styled-map-package.js

+// @ts-expect-error Need to publish types for module
+import { Reader } from 'styled-map-package'
+
+/** @import {FastifyPluginAsync} from 'fastify' */
+// @ts-expect-error Need to publish types for module
+/** @import {Resource} from 'styled-map-package/reader' */
+// @ts-expect-error Need to publish types for module
+/** @import {SMPStyle} from 'styled-map-package/types' */


Just noting that these ignores are allowing tests to run without immediately failing

achou11 · 2024-10-02T18:47:39Z

src/fastify-plugins/maps/index.js

+      // 1. Attempt to use the styled map package
+      {
+        const style = await fastify.comapeoSmp.getStyle().catch(() => {
+          fastify.log.warn('Cannot read styled map package archive')
+          return null
+        })
+
+        if (style) {
+          return style
+        }
+      }


I made the assumption that we want to give highest precedence to an SMP if possible

achou11 · 2024-10-03T16:07:28Z

One thing that this PR doesn't fully address is this requirement listed in the issue:

based on app requirements, we'll need the following information about an SMP map:

date added

storage used

name

Not really sure what the best way to expose this information will be.

storage used = easy to do client side
date added = technically can do client side
name = not really sure what the best thing to do here is. Is this something that should be derived from the style.json within the SMP, or can it just be based on the name of the SMP file? My guess is that it shouldn't be the latter, which makes the implementation a little trickier to figure out.

Maybe all of this is out of scope for this PR. In which case I can either:

update the PR description so that it doesn't close the issue. A create a follow-up PR that addresses this specific requirement that officially closes the issue.
extract that portion out of the issue into a separate issue.

awana-lockfile-bot · 2024-10-03T17:43:24Z

`package-lock.json` changes

Summary

Status	Count
	74
	9

Click to toggle table visibility

Name	Previous	Current
`@fastify/static`	7.0.3	7.0.4
`@mapbox/jsonlint-lines-primitives`	-	2.0.2
`@mapbox/sphericalmercator`	-	1.2.0
`@mapbox/unitbezier`	-	0.0.1
`@maplibre/maplibre-gl-style-spec`	-	20.3.1
`@placemarkio/check-geojson`	-	0.1.12
`@turf/bbox`	-	7.1.0
`@turf/helpers`	-	7.1.0
`@turf/meta`	-	7.1.0
`@types/geojson`	-	7946.0.14
`ansi-diff`	-	1.2.0
`ansi-split`	-	1.0.1
`archiver-utils`	-	5.0.2
`archiver`	-	7.0.1
`arr-union`	-	3.1.0
`assign-symbols`	-	1.0.0
`async`	-	3.2.6
`buffer-peek-stream`	-	1.1.0
`bundle-name`	-	4.1.0
`bytewise-core`	-	1.2.3
`bytewise`	-	1.1.0
`cli-spinners`	-	2.9.2
`clone`	-	1.0.4
`compress-commons`	-	6.0.2
`core-util-is`	-	1.0.3
`crc-32`	-	1.2.2
`crc32-stream`	-	6.0.0
`default-browser-id`	-	5.0.0
`default-browser`	-	5.2.1
`defaults`	-	1.0.4
`define-lazy-prop`	-	3.0.0
`extend-shallow`	-	2.0.1
`fastify`	4.26.2	4.28.1
`filter-obj`	-	6.1.0
`get-east-asian-width`	-	1.2.0
`get-value`	-	2.0.6
`into-stream`	-	8.0.1
`is-docker`	-	3.0.0
`is-extendable`	-	0.1.1
`is-inside-container`	-	1.0.0
`is-interactive`	-	2.0.0
`is-plain-object`	-	2.0.4
`is-unicode-supported`	-	2.1.0
`is-wsl`	-	3.1.0
`isobject`	-	3.0.1
`json-stringify-pretty-compact`	-	4.0.0
`ky`	-	1.7.2
`lazystream`	-	1.0.1
`log-symbols`	-	7.0.0
`mimic-function`	-	5.0.1
`minimist`	1.2.7	1.2.8
`normalize-path`	-	3.0.0
`open`	-	10.1.0
`ora`	-	8.1.0
`package-json-from-dist`	-	1.0.1
`parse-ms`	-	4.0.0
`pino-abstract-transport`	1.1.0	1.2.0
`pino-std-serializers`	6.2.2	7.0.0
`pino`	8.20.0	9.4.0
`pretty-bytes`	-	6.1.1
`pretty-ms`	-	9.1.0
`process-nextick-args`	-	2.0.1
`quickselect`	-	2.0.0
`readdir-glob`	-	1.1.3
`run-applescript`	-	7.0.0
`rw`	-	1.3.3
`safe-stable-stringify`	2.4.3	2.5.0
`set-value`	-	2.0.1
`sonic-boom`	3.8.1	4.1.0
`sort-asc`	-	0.2.0
`sort-desc`	-	0.2.0
`sort-object`	-	3.0.3
`split-string`	-	3.1.0
`stdin-discarder`	-	0.2.2
`styled-map-package`	-	1.0.1
`thread-stream`	2.4.1	3.1.0
`tinyqueue`	-	3.0.0
`typewise-core`	-	1.2.0
`typewise`	-	1.0.3
`union-value`	-	1.0.1
`wcwidth`	-	1.0.1
`yoctocolors`	-	2.1.1
`zip-stream`	-	6.0.1

awana-lockfile-bot · 2024-10-04T16:12:34Z

`package-lock.json` changes

Summary

Status	Count
	74
	9

Click to toggle table visibility

Name	Previous	Current
`@fastify/static`	7.0.3	7.0.4
`@mapbox/jsonlint-lines-primitives`	-	2.0.2
`@mapbox/sphericalmercator`	-	1.2.0
`@mapbox/unitbezier`	-	0.0.1
`@maplibre/maplibre-gl-style-spec`	-	20.3.1
`@placemarkio/check-geojson`	-	0.1.12
`@turf/bbox`	-	7.1.0
`@turf/helpers`	-	7.1.0
`@turf/meta`	-	7.1.0
`@types/geojson`	-	7946.0.14
`ansi-diff`	-	1.2.0
`ansi-split`	-	1.0.1
`archiver-utils`	-	5.0.2
`archiver`	-	7.0.1
`arr-union`	-	3.1.0
`assign-symbols`	-	1.0.0
`async`	-	3.2.6
`buffer-peek-stream`	-	1.1.0
`bundle-name`	-	4.1.0
`bytewise-core`	-	1.2.3
`bytewise`	-	1.1.0
`cli-spinners`	-	2.9.2
`clone`	-	1.0.4
`compress-commons`	-	6.0.2
`core-util-is`	-	1.0.3
`crc-32`	-	1.2.2
`crc32-stream`	-	6.0.0
`default-browser-id`	-	5.0.0
`default-browser`	-	5.2.1
`defaults`	-	1.0.4
`define-lazy-prop`	-	3.0.0
`extend-shallow`	-	2.0.1
`fastify`	4.26.2	4.28.1
`filter-obj`	-	6.1.0
`get-east-asian-width`	-	1.2.0
`get-value`	-	2.0.6
`into-stream`	-	8.0.1
`is-docker`	-	3.0.0
`is-extendable`	-	0.1.1
`is-inside-container`	-	1.0.0
`is-interactive`	-	2.0.0
`is-plain-object`	-	2.0.4
`is-unicode-supported`	-	2.1.0
`is-wsl`	-	3.1.0
`isobject`	-	3.0.1
`json-stringify-pretty-compact`	-	4.0.0
`ky`	-	1.7.2
`lazystream`	-	1.0.1
`log-symbols`	-	7.0.0
`mimic-function`	-	5.0.1
`minimist`	1.2.7	1.2.8
`normalize-path`	-	3.0.0
`open`	-	10.1.0
`ora`	-	8.1.0
`package-json-from-dist`	-	1.0.1
`parse-ms`	-	4.0.0
`pino-abstract-transport`	1.1.0	1.2.0
`pino-std-serializers`	6.2.2	7.0.0
`pino`	8.20.0	9.4.0
`pretty-bytes`	-	6.1.1
`pretty-ms`	-	9.1.0
`process-nextick-args`	-	2.0.1
`quickselect`	-	2.0.0
`readdir-glob`	-	1.1.3
`run-applescript`	-	7.0.0
`rw`	-	1.3.3
`safe-stable-stringify`	2.4.3	2.5.0
`set-value`	-	2.0.1
`sonic-boom`	3.8.1	4.1.0
`sort-asc`	-	0.2.0
`sort-desc`	-	0.2.0
`sort-object`	-	3.0.3
`split-string`	-	3.1.0
`stdin-discarder`	-	0.2.2
`styled-map-package`	-	1.0.1
`thread-stream`	2.4.1	3.1.0
`tinyqueue`	-	3.0.0
`typewise-core`	-	1.2.0
`typewise`	-	1.0.3
`union-value`	-	1.0.1
`wcwidth`	-	1.0.1
`yoctocolors`	-	2.1.1
`zip-stream`	-	6.0.1

gmaclennan

I had a read through this today. This is the first time I am reading the code from the maps plugin, so some comments are not directly related to this PR, but we may want to consider them when implementing this. I'll try my best to separate the two, but some decisions in the current implementation mean that we need to do a lot more work here, so it may be worth while fixing those now rather than doing extra work.

PR specific comments

I do recommend using the styled-map-package server plugin. With the latest fixes I think it should do everything needed in a self-contained way, and if it doesn't we should fix that upstream.

For the client requirements of date added, storage used, and name, I think we should add a route outside the plugin. We don't need access to the Reader instance for it, we can do something like:

fastify.get('/smp/info', async (req, reply) => {
  const stats = await fs.stat(smpFilepath)
  const style = await fetch('/smp/style.json').json()
  return {
    created: stats.ctime,
    size: stats.size
    name: style.name || path.parse(smpFilepath).name
  }
})
fastify.register(smpPlugin)

For selecting a style to server based on availability we have a few options:

Either use decorator methods or fetch upstream to load style.json into memory, then return that.
No decorator methods, and proxy everything (local or remote) either with our own fetch or with @fastify/reply-from.
HEAD requests then 302 redirects.

My preference is (3) because I think it's the simplest (pseudo-code):

fastify.get('style.json', async (req, reply) => {
  const res = await fetch({ method: 'HEAD', url: styleUrl }).catch(noop)
  if (res && res.status === 200) reply.status(302).setHeader('location', styleUrl)
})

Architecture opinions

I think we should just drop static-maps right now. We haven't released anything that supports this, and we haven't communicated any support for it, and keeping it just adds maintenance burden and the need to handle backwards compatibility.
I think we're making things hard for ourselves by exposing each of the "sub-plugins" outside of this module - we're leaking our implementation details. I think the maps plugin should be a single plugin. Otherwise we don't control the order the sub-plugins are registered, and we need to reach into internals by decorating everything.
Why not register the maps plugin in Mapeo Manager rather than export it as a separate thing?
Rather than trying to decorate the plugin with the getStyleJsonUrl() method, maybe move that into MapeoManager like with the blob and icon servers (easy to do if registering internally)
I think it's worth using styled map package for the fallback map too, to simplify things, see https://github.com/digidem/comapeo-fallback-smp

There may well be very valid reasons for not choosing to do things this way. I'm happy to jump on a quick call about it, rather than you needing to write down a justification for everything here.

achou11 · 2024-10-07T11:38:36Z

@gmaclennan thanks for the feedback! i think it all makes sense and I honestly don't have any strong arguments against the suggestions regarding architecture. At the time I initially worked on the maps plugin, I wasn't aware of the options for style selection that you've outlined, which is how i ended up with this weird setup 😄

fully agree that I would just re-use the smp fastify plugin from upstream, i just didn't want to be blocked on the issues I originally encountered (since you were focusing on other things). Those have been addressed at this point (once published) so I'll need to update the PR with that.

As for updating the general implementation of the maps plugin, would you suggest I do that in a separate PR that this PR stacks on top of? feels like trying to do that work after updating and merging this one would be annoying vs the other way around

achou11 · 2024-10-08T13:45:45Z

Closing this PR in favor of a completely new one that will significantly simplify how the maps plugin works, based on the suggestions provided above

achou11 force-pushed the 827/serve-smp branch 6 times, most recently from 057f5ef to 94c825d Compare October 2, 2024 16:22

initial implementation

0ebc4ef

achou11 force-pushed the 827/serve-smp branch from 94c825d to 0ebc4ef Compare October 2, 2024 16:25

digidem deleted a comment from awana-lockfile-bot bot Oct 2, 2024

achou11 added 2 commits October 2, 2024 13:42

ignore type errors for now

f4b59dc

refine tests and add fixture

68e157e

achou11 changed the title ~~WIP: feat: support serving Style Map Package maps~~ feat: support serving Style Map Package maps Oct 2, 2024

achou11 commented Oct 2, 2024

View reviewed changes

achou11 marked this pull request as ready for review October 2, 2024 18:48

achou11 requested a review from gmaclennan October 2, 2024 18:48

improve basic resource fetching test

7a9fee7

fix some asserts

60e70d0

gmaclennan reviewed Oct 7, 2024

View reviewed changes

achou11 closed this Oct 8, 2024

achou11 deleted the 827/serve-smp branch October 8, 2024 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support serving Style Map Package maps #881

feat: support serving Style Map Package maps #881

achou11 commented Oct 1, 2024 •

edited

Loading

awana-lockfile-bot bot commented Oct 2, 2024

awana-lockfile-bot bot commented Oct 2, 2024

achou11 Oct 2, 2024

achou11 Oct 2, 2024

achou11 commented Oct 3, 2024 •

edited

Loading

awana-lockfile-bot bot commented Oct 3, 2024

awana-lockfile-bot bot commented Oct 4, 2024

gmaclennan left a comment

achou11 commented Oct 7, 2024

achou11 commented Oct 8, 2024

feat: support serving Style Map Package maps #881

feat: support serving Style Map Package maps #881

Conversation

achou11 commented Oct 1, 2024 • edited Loading

awana-lockfile-bot bot commented Oct 2, 2024

package-lock.json changes

Summary

awana-lockfile-bot bot commented Oct 2, 2024

package-lock.json changes

Summary

achou11 Oct 2, 2024

Choose a reason for hiding this comment

achou11 Oct 2, 2024

Choose a reason for hiding this comment

achou11 commented Oct 3, 2024 • edited Loading

awana-lockfile-bot bot commented Oct 3, 2024

package-lock.json changes

Summary

awana-lockfile-bot bot commented Oct 4, 2024

package-lock.json changes

Summary

gmaclennan left a comment

Choose a reason for hiding this comment

achou11 commented Oct 7, 2024

achou11 commented Oct 8, 2024

achou11 commented Oct 1, 2024 •

edited

Loading

`package-lock.json` changes

`package-lock.json` changes

achou11 commented Oct 3, 2024 •

edited

Loading

`package-lock.json` changes

`package-lock.json` changes