feat: add serverAddress to request handler
#308
Conversation
|
|
||
| next() | ||
| }) | ||
| logger.log(`Middleware loaded. Emulating features: ${netlifyDev.getEnabledFeatures().join(', ')}.`) |
There was a problem hiding this comment.
I don't see the value of this. It won't mean anything to anyone using the Vite plugin through a meta-framework.
There was a problem hiding this comment.
Could you move this change to a separate PR so we can discuss it separately?
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
LGTM - just would rather decouple the logging change
| /** | ||
| * If your local development setup has its own HTTP server (e.g. Vite), you | ||
| * can supply its address here. It will override any value defined in the | ||
| * top-level `serverAddress` setting. | ||
| */ | ||
| serverAddress?: string |
There was a problem hiding this comment.
suggestion: document in packages/dev/README.md
packages/dev/src/main.ts
Outdated
| } | ||
|
|
||
| if (this.#server instanceof HTTPServer) { | ||
| return await this.#server.start() |
There was a problem hiding this comment.
question: it's surprising that a getServerAddress method has the behaviour/side-effect of starting a server. is there an easy way to avoid that?
e.g. I would expect that it would be idempotent but it will throw on subsequent calls (I think)
There was a problem hiding this comment.
agreed. i wonder if HTTPServer should have a .serving() promise to await here, and #server.start() should be run somewhere else? or, if this is definitely the right place then perhaps a more dramatic verb than get
There was a problem hiding this comment.
We don't actually need to start a server in getServerAddress. I think it may have been a transient state while developing this that I forgot to clean up.
Fixed in ecef1bf.
Currently,
NetlifyDevaccepts aserverAddressoption in the constructor. When used in the Vite plugin, this property needs to match the address of the Vite server. This is a problem in some Vite-based frameworks (like Astro), because when we initialise the plugin, the server isn't running yet, so we don't know what port it uses.With this PR, we're able to defer the definition of
serverAddressto thehandlemethod. At that point, we know for a fact that the Vite server is already running, so we grab the port and set it on thehandleoptions.I have kept the
serverAddressoption in the constructor for backwards-compatibility; if both are supplied, the one inhandletakes precedence.