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

feat: support serving static maps #435

Merged
merged 12 commits into from
Mar 12, 2024
Merged

feat: support serving static maps #435

merged 12 commits into from
Mar 12, 2024

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Jan 16, 2024

Closes #437

Towards #439

Stacked on #440

Combines changes extracted from digidem/mapeo-map-server#101 and digidem/comapeo-mobile#155

Notes:

src/fastify-plugins/static-maps.js Outdated Show resolved Hide resolved
src/media-server.js Outdated Show resolved Hide resolved
@achou11 achou11 force-pushed the ac/serve-static-maps branch 2 times, most recently from 7f10e2b to 3df04bd Compare January 18, 2024 20:46
src/fastify-plugins/maps/index.js Show resolved Hide resolved
src/mapeo-manager.js Outdated Show resolved Hide resolved
src/media-server.js Outdated Show resolved Hide resolved
@achou11 achou11 changed the base branch from main to ac/media-server-refactor January 23, 2024 18:15
src/fastify-plugins/maps/index.js Outdated Show resolved Hide resolved
Comment on lines +228 to +243
// TODO: If necessary, need to flip the y value first if the TileJSON source is TMS scheme
// Doing this will depend on if we decide that the asar directory structure should only follow XYZ or if it should align with corresponding tilejson spec
Copy link
Member Author

@achou11 achou11 Jan 23, 2024

Choose a reason for hiding this comment

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

highlighting this. are we able to assume that the tile urls use z/x/y format and the directories that are stored on the filesystem follow that convention? basically, how much control do we usually have over the style.json files that are used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not my most helpful comment, but want to explicitly say: I don't know. My hunch is that we should get an answer to this before the MVP, but not before merging this.

@achou11 achou11 force-pushed the ac/serve-static-maps branch 5 times, most recently from b391b3d to 24361ec Compare January 24, 2024 21:57
Base automatically changed from ac/media-server-refactor to main January 24, 2024 22:00
src/fastify-plugins/maps/index.js Outdated Show resolved Hide resolved
src/fastify-plugins/maps/static-maps.js Outdated Show resolved Hide resolved
Comment on lines 50 to 52
// TODO: Currently fails
// Requires fixture setup that has default/ in static directory, online proxy, then offline fallback map
skip('/style.json resolves with expected style.json', async (t) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

will enable this test when working on #439

tests/fastify-plugins/static-maps.js Outdated Show resolved Hide resolved
tests/fastify-plugins/static-maps.js Outdated Show resolved Hide resolved
tests/fastify-plugins/static-maps.js Outdated Show resolved Hide resolved
tests/fastify-plugins/static-maps.js Outdated Show resolved Hide resolved
tests/fastify-plugins/maps.js Show resolved Hide resolved
tests/fastify-plugins/maps.js Show resolved Hide resolved
tests/fastify-plugins/static-maps.js Show resolved Hide resolved
src/fastify-plugins/maps/index.js Show resolved Hide resolved
src/fastify-plugins/maps/static-maps.js Show resolved Hide resolved
src/fastify-plugins/maps/index.js Show resolved Hide resolved
@EvanHahn
Copy link
Contributor

Had the following thought while I was reviewing: you've split this into multiple Fastify plugins. Why not just have one big plugin? I worry this is a premature abstraction...are there ever cases where you wouldn't include all the plugins?

@achou11
Copy link
Member Author

achou11 commented Mar 11, 2024

Had the following thought while I was reviewing: you've split this into multiple Fastify plugins. Why not just have one big plugin? I worry this is a premature abstraction...are there ever cases where you wouldn't include all the plugins?

could categorize as premature but useful (also easier to build iteratively since they're all rather distinct functionalities).

the idea was to build this in a way that could potentially be split off into its own package that contais all of these plugins. to summarize the purpose of each plugin:

  • maps: mostly specific to the "new" maps serving approach for Mapeo. the initial implementation uses a decorator exposed by static maps plugin but that's to support backward compatibility for how maps are stored on a device (since the map manager isn't implemented for comapeo yet). eventually, this will primarily use the JS API for the map server dep that you've been working on, and expose the routes necessary to power the Mapeo Map Manager.

  • static-maps: adding functionality for the "legacy" map serving approach that Mapeo uses i.e. an embedded directory with a specific structure and some metadata files that get served statically. this is isolated from the new approach but we need the functionality for backwards compat reasons to ease the transition.

  • offline-map: very specific to how mapeo works - probably didn't need to be its own plugin but made it easier to isolate the implementation

are there ever cases where you wouldn't include all the plugins?

if we ever fully move away from the static directory approach of serving maps, then it'll be easier to remove that piece with the plugin approach implemented here. so in theory, yes - the static-maps plugin could potentially cease to exist if we ever reach that point 😄

@achou11 achou11 merged commit 16e9547 into main Mar 12, 2024
4 checks passed
@achou11 achou11 deleted the ac/serve-static-maps branch March 12, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for serving custom offline maps
2 participants