feat: support glyphs #54

gmaclennan · 2022-08-01T11:22:44Z

Closes #5

@achou11 here's a quick script to generate SDFs - just wanted to check it worked and start my Monday with something with a quick result :)

achou11

if it works, I won't dispute that 😄 had a couple of small comments but otherwise seems safe to merge and use in a follow up

scripts/generate-sdf.js

achou11 · 2022-08-01T13:57:09Z

scripts/generate-sdf.js

+
+async function generateSdf({ font, outDir }) {
+  return new Promise((resolve, reject) => {
+    for (let i = 0; i < 65536; i = i + 256) {


for my own education, what are these numbers?

These number are character codes. The mapbox api expects SDFs to be served as combined files of 256 glyphs (see the API details). For ASCII the map only needs to request the 0-255.pbf, but when the client library (mapbox-gl-js) comes across labels with other characters, it downloads the glyph ranges needed to render those characters.

ah I see. maybe worth putting a link to the docs here but thanks for explaining!

achou11 · 2022-08-01T14:00:31Z

just noticed ci failed and that's because fontnik doesn't support node 16. not necessarily blocking since it's only being used as a dev dependency, but worth noting before merging

achou11 · 2022-08-03T20:56:38Z

@gmaclennan marked as ready for review. probably missing a few key tests and some refinements, but think it's ready to be glanced at

achou11 · 2022-08-03T20:58:00Z

src/api/errors.ts

+export const createForwardedUpstreamError = (statusCode: number) =>
+  createError(
+    `FORWARDED_UPSTREAM_${statusCode}`,
+    'Upstream request at %s responded with: %s',
+    statusCode
+  )


There may be a better way of approaching and/or naming this, but the idea is to be able to forward errors that we receive from upstream endpoints

achou11 · 2022-08-03T21:01:23Z

src/api/errors.ts

@@ -27,7 +27,7 @@ export const MismatchedIdError = createError(
 export const MBAccessTokenRequiredError = createError(
  'FST_ACCESS_TOKEN',
  'A Mapbox API access token is required for styles that use Mapbox-hosted sources',
-  400
+  401


Changed this to match the status code used by Mapbox's API for errors related to the access token, which is consistent across their different resources

achou11 · 2022-08-03T21:11:07Z

src/api/glyphs.ts

+  // TODO: Use LRU for this?
+  function getStaticFile(font: string, start: number, end: number) {


would using LRU help with this?

achou11 · 2022-08-03T21:30:48Z

src/api/glyphs.ts

+        // TODO: Change the kind of error thrown here?
+        if (!row) throw err


highlighting this

achou11 · 2022-08-03T21:31:51Z

src/api/glyphs.ts

+
+        // 2. Offline attempt failed, but may be able to get upstream resource
+
+        // TODO: Validate that the glyphs url contains {fontstack} and {range} templates?


highlighting this

achou11 · 2022-08-03T21:34:35Z

src/lib/glyphs.ts

+
+// Returns an array of values where each value is a comma-separated string of font names
+// Adapted version of https://github.com/digidem/mapbox-style-downloader/blob/695ed8a981efb9f0ece80cba8c81d075f9a0cdda/lib/glyphs.js#L21-L53
+export function getFontStacks(style: StyleJSON): string[] {


this is not used at the moment

achou11 · 2022-08-03T21:35:24Z

src/routes/fonts.ts

+          err instanceof NotFoundError ||
+          (err instanceof HTTPError && err.response.statusCode === 404)
+
+        // TODO: Do we want to return default fallback if upstream is not found?


highlighting this

achou11 · 2022-08-03T21:44:19Z

src/routes/fonts.ts

+        // This is when the upstream api provides an error status
+        if (err instanceof HTTPError) {
+          throw new (createForwardedUpstreamError(err.response.statusCode))(
+            err.response.url,
+            err.response.statusMessage
+          )
+        }


seems useful to forward the error response from the upstream api in this case - i'd imagine that the map client making these requests would expect an api similar to the mapbox one

achou11 · 2022-08-03T21:50:37Z

src/api/glyphs.ts

+    // TODO: Should we return always return the offline asset if it exists?
+    async getGlyphs({ styleId, accessToken, font, start, end }) {


the approach implemented is:

Find the matching offline asset.
a. If it exists, attempt to return that static asset
b. if it doesn't exist: go to step 2

Fetch from upstream.
a. if successful, return the response from upstream
b. if not successful because:

resource not found (404), attempt to return the fallback offline asset (Open Sans)

some other other response error, forward that error

thinking that we probably need to handle a case where we know we're offline and therefore should serve up the fallback offline asset at the very least

see 13f0bf3 for offline handling. in this case we serve the default static asset

initial build script to generate SDFs

053dd7e

achou11 approved these changes Aug 1, 2022

View reviewed changes

achou11 added 5 commits August 1, 2022 11:01

add license file to font

ca5ed15

install @fastify/static@5

34125da

Merge branch 'master' into feat/glyphs

e717590

output pbf files for sdf script

ae2f4e6

set up @fastify/static plugin

c1b3917

achou11 self-requested a review August 1, 2022 15:48

achou11 added 21 commits August 1, 2022 12:48

update type annotation for createServer

5954745

add basic route and tests for fonts

fcf8570

remove unused imports

15f0414

update fonts route to handle fontstack param better

e7f2045

replace style's glyphs field with offline url

ae359ae

update create style test to check change in glyphs field

0b995b5

remove only call

b425cfd

generate sdfs in ci

20ee781

remove console log in test

d43631f

basic api implementation of getGlyphs

313b33a

add more half-baked tests

f9cc35f

implement untested upstream glyphs functionality

2a8dffe

remove comment

959d904

update naming convention for statically stored fonts

0cf0b1b

add todo about validating upstream url

ea23bdd

update variable name

fd247c8

fix glyph url generation

8b465b2

update access token error status code to match mapbox

bc33d47

forward upstream responses

f38d819

fix mock in test

35abd03

fix git naming of open sans dir

3c1ba7c

update comments in fastify static register

bf69fc6

achou11 marked this pull request as ready for review August 3, 2022 20:55

set etag from upstream to reply headers

9eb6962

achou11 reviewed Aug 3, 2022

View reviewed changes

achou11 added 6 commits August 4, 2022 13:10

return error for invalid ranges

7c9ed4b

add test for error response forwarding

bd905e6

reduce boilerplate in glyphs tests

65272d9

handle offline case

13f0bf3

fix comment

e286bd2

move offline error codes to errors file

0a3e82d

achou11 mentioned this pull request Aug 8, 2022

Support Vector Tiles #49

Closed

6 tasks

gmaclennan mentioned this pull request Aug 8, 2022

feat: HTML map previews #55

Merged

disable ci for node 16 due to node-fontnik

2ced2e4

achou11 merged commit c0d0231 into master Aug 8, 2022

achou11 deleted the feat/glyphs branch August 8, 2022 17:20

achou11 mentioned this pull request Aug 15, 2022

Enable Node 16 for CI #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support glyphs #54

feat: support glyphs #54

gmaclennan commented Aug 1, 2022 •

edited by achou11

Loading

achou11 left a comment

achou11 Aug 1, 2022

gmaclennan Aug 1, 2022

achou11 Aug 1, 2022

achou11 commented Aug 1, 2022

achou11 commented Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 3, 2022

achou11 Aug 4, 2022

		// TODO: Use LRU for this?
		function getStaticFile(font: string, start: number, end: number) {

		// TODO: Change the kind of error thrown here?
		if (!row) throw err


		// 2. Offline attempt failed, but may be able to get upstream resource

		// TODO: Validate that the glyphs url contains {fontstack} and {range} templates?

		// TODO: Should we return always return the offline asset if it exists?
		async getGlyphs({ styleId, accessToken, font, start, end }) {

feat: support glyphs #54

feat: support glyphs #54

Conversation

gmaclennan commented Aug 1, 2022 • edited by achou11 Loading

achou11 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

achou11 commented Aug 1, 2022

achou11 commented Aug 3, 2022

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

gmaclennan commented Aug 1, 2022 •

edited by achou11

Loading