chore!: refactor MapeoManager to accept Fastify server instance

[achou11]

Towards #437

As I'm working on #437 and dealing with more Fastify instantiation and plugins, I'm noticing that there's an ergonomic awkwardness caused by the MapeoManager being responsible for instantiating the media server instance. Basically was becoming more and more prone to defining MediaServer-specific options for the MapeoManager constructor, which didn't make much sense since the manager mostly just passes it along to the media server.

The ideal approach is only needing to provide an instance of the MediaServer to instantiate the MapeoProject. I initially thought that this wasn't possible because some of the plugins we register for the media server are dependent on methods provided by the manager. The flow of operations was roughly:

1. Instantiate MapeoManager
2. Manager instantiates MediaServer internally
3. MediaServer internally registers plugins dependent on MapeoManager methods

Then I realized that even if that's the case, those methods are only needed when we register the relevant plugins. With this in mind, this PR updates the MediaServer and MapeoManager such that we can now have the following flow of operations instead:

1. Instantiate MediaServer
2. Instantiate MapeoManager, providing instance of MediaServer
3. MapeoManager registers plugins dependent on MapeoManager methods

This makes it much easier to integrate new plugins. In the context of the maps-related functionality, none of the implementation is reliant on the MapeoManager so it can be registered outside of the MapeoManager more freely, meaning we don't need to specify plugin-specific opts for the manager that are just relayed. E.g.

const mediaServer = new MediaServer()
// Will register Blobs and Icons plugins internally
const manager = new MapeoManager( { mediaServer, ... })

// Can register manager independent plugins wherever and whenever!
mediaServer.registerPlugin(MapsPlugin, ...)

---

Remaining questions:

• Do we need MapeoManager.startMediaServer() and MapeoManager.stopMediaServer() anymore with this change? Those are just exposing the relevant media server methods but if the media server is available outside the context of the manager, could we just defer to using mediaServer.start() and mediaServer.close()? EDIT: no longer needed
• Should we rename the MediaServer at this point? Given this note in Add support for serving custom offline maps  #437: DONE: renamed to FastifyController and reduced in scope

Rename the MediaServer to something more generic. Maybe something like MapeoHttpServer, ResourceServer, AppServer??

Notes:

• Now that the HTTP server is instantiated separately, consumers of this package will need to instantiate the server themselves and then provide that when instantiating the manager. Should probably update any relevant docs with that

[gmaclennan]

This makes a lot of sense, I like how it simplifies things. I'm tempted to go even further, and just make the parameter for MapeoManager to be a fastify instance. Then the getServerAddress(fastify.server) can just be defined in Mapeo code, and what is called MediaServer could just be FastifyController or FastifyStopStart - could break into a separate module if we want. getMediaAddress could just be a private method on MapeoManager, or we could wrap the logic into MapeoProject methods.

I don't think we need to export start/stopMediaServer on the Manager instance - we start and stop the server from the backend, so it doesn't need to be exposed in the API.

Could make a fastify instance a param of FastifyController too, so code would be:

const fastify = Fastify({ logger })
const fastifyController = new FastifyController({ fastify })
const manager = new MapeoManager({ fastify, ... })
fastifyController.start()

A bit verbose maybe, but clear.

[achou11]

@gmaclennan yep makes a lot of sense! will incorporate that suggestion into this PR (won't create a separate module for now)

[achou11]

@gmaclennan made the following changes based on your feedback:

• renamed MediaServer to FastifyController
• updated MapeoManager to:
  • accept Fastify instance as constructor param instead of MediaServer instance
  • implement private getMediaAddress() method (used to live in MediaServer)
• update relevant tests (file names and tests)
• renamed and updated docs

[gmaclennan]

This looks great, much neater I think than what we had before. The few things I noticed are actually things from before this PR, it's just that reviewing the code I noticed them. Mainly just nit-picks, but there is one thing that should be addressed - the setting of isFastifyStarted which should happen before fastify.listen() to avoid the race condition when the server is started twice in quick succession. Other suggestions aren't blocking.

[gmaclennan]

I think maybe add something to this about this being optional - on Desktop we would only need to call fastify.listen(), the use of FastifyController is only necessary for mobile where we need to start and stop the server (because Android doesn't like us running a server while the app is in the background)

[achou11]

addressed via 3fb05dd

[gmaclennan]

I know this was copied across - I realise there is a bug here from my original code. This should avoid a race condition where calling startServer() twice before fastify.listen() resolves would throw an error.

Suggested change

	await this.#fastify.listen({ host: this.#host, port: this.#port })
	this.#fastifyStarted = true
	this.#fastifyStarted = true
	await this.#fastify.listen({ host: this.#host, port: this.#port })

[achou11]

addressed via a772a42

[gmaclennan]

Nit: Again, something carried over, but I don't think these need to be instance properties - they are only used within this method.

[achou11]

addressed via f80ac9a

[gmaclennan]

Again, something from before, which I'm only noticing because I'm reviewing it. I think how it is fine and won't cause issues, but this change makes it a bit more "correct".

Wrapping this function in a pTimeout leaves the once(server, 'listening') hanging. Better to integrate timeout into this function. I only just learnt about AbortSignal.timeout() but it's helpful here.

Suggested change

	* @param {import('node:http').Server} server
	* @returns {Promise<string>}
	*/
	export async function getFastifyServerAddress(server) {
	const address = server.address()
	if (!address) {
	await once(server, 'listening')
	* @param {import('node:http').Server} server
	* @param {{ timeout?: number }} [options]
	* @returns {Promise<string>}
	*/
	export async function getFastifyServerAddress(server, { timeout } = {}) {
	const address = server.address()
	if (!address) {
	await once(server, 'listening', { signal: timeout ? AbortSignal.timeout(timeout) : undefined })

[achou11]

addressed via 3aafbc0

[gmaclennan]

Nit:

Suggested change

	async #getMediaAddress(mediaType) {
	async #getMediaBaseUrl(mediaType) {

[achou11]

addressed via 29727df

[gmaclennan]

if implementing the suggestion above. Also maybe 5 second timeout is safer (in case on a slow phone it takes a while for the server to start)

Suggested change

	const base = await pTimeout(getFastifyServerAddress(this.#fastify.server), {
	milliseconds: 1000,
	})
	const base = await getFastifyServerAddress(this.#fastify.server, {
	timeout: 5000,
	})

[achou11]

addressed via 3aafbc0