Open jtoar opened 2 years ago
Hey @jtoar I hope you don't mind that I started looking into this. I'd say I'm about 90% the way there with the fix but wanted to clarify some of the other existing behaviour which looks questionable to me.
The options passed to the api
or web
or both are handled by:
https://github.com/redwoodjs/redwood/blob/fbfd467613fe28eeca429bd0f491ccf0ed80bdca/packages/api-server/src/cliHandlers.ts#L21-L46
In principle the fix to this specific issue is the addition of something like:
host: { default: getConfig().api?.host, type: 'string', alias: 'h' }
into apiCliOptions
, where the default value (host = process.env.NODE_ENV === 'production' ? '0.0.0.0' : '::'
) is handled with a default value in startServer
- like the port is:
https://github.com/redwoodjs/redwood/blob/fbfd467613fe28eeca429bd0f491ccf0ed80bdca/packages/api-server/src/server.ts#L9-L13
So that appears to me to be the fix needed.
However, when I start to work through how these config options flow through to bothServerHandler
then it leads me to ask:
startFastifyServer
takes one host - but what if the user specifies both a web.host
and api.host
? In principle they can be different and what one do we choose? If we choose one over the other the user will need to know.port
- user could provide two and we only accept one but which?yarn rw serve
the output is:
Web server started on http://localhost:8910
API serving from http://localhost:8910
Both services running on the same port, is that an issue?
These issue's don't exist with the apiServerHandler
or webServerHandler
because we can specifically take whatever config is appropriate.
If this is better suited to a PR which solves this problem and then a separate issue/PR to address the wider bothServerHandler
then let me know 😄
Hey @Josh-Walker-GM, sorry for the late response. You hit the nail on the head. It actually surprised me that they run on the same port. I don't usually work on the api server so asking the team for a bit of context now.
If you're running both servers via yarn rw serve
, it seems like there would have to be two options like --web.host
and --api.host
(same for port). Yargs actually parses arguments with dots in their names into an object, so that part shouldn't be too bad: https://github.com/yargs/yargs/blob/main/docs/tricks.md#objects. But to answer your question, I don't think we choose between web.host
and api.host
, unless there's something I don't understand (i.e. they have to run on the same host for some reason).
--host
and --port
flags for yarn rw serve
, which would be breaking
NODE_ENV
env var and what it's role isAssigning myself just for triage. Your welcome to work on the PR but I'd probably wait till I get more clarity on the setup, just to make sure my reasoning is on track.
No worries about the response time at all! I'm more than happy to wait and see if some more information is added to the issue before going any deeper into a full PR - I'm looking to wrap up my existing PRs right now before opening too many new ones anyway.
One thing I will add though is that it appears to me that when you run yarn rw serve
which filters down into calling bothServerHandler
that you only actually create one server. My expectations before looking into the code was that bothServerHandler
would act as a wrapper which performs some config/setup but ultimately starts two servers one api and one web but this isn't my understanding of what the code does.
This can be seen where the bothServerHandler
calls startServer
:
https://github.com/redwoodjs/redwood/blob/5cef05cb4b21d822d64c08bd4fc6a5b6927dc3ab/packages/api-server/src/cliHandlers.ts#L90-L94
And what this startServer
actually does:
https://github.com/redwoodjs/redwood/blob/5cef05cb4b21d822d64c08bd4fc6a5b6927dc3ab/packages/api-server/src/server.ts#L9-L28
When I read this it looks to me like we start one server?
The reason it appears to work is because both the api functions and the web serve files are attached to the same server: https://github.com/redwoodjs/redwood/blob/5cef05cb4b21d822d64c08bd4fc6a5b6927dc3ab/packages/api-server/src/cliHandlers.ts#L86-L88
I may be wrong but this is just my understanding after reading through. It's cool to be able to only need one server to do both but it then complicates how you deal with config options which are geared towards two separate servers.
@jtoar I know you're busy with other matters. You mentioned others on the team more familiar with this area of the codebase. Are any of them able to provide input on this?
What's not working?
The docs say you can configure the host of the api server (see https://redwoodjs.com/docs/app-configuration-redwood-toml#api). But if you specify api.host in the
redwood.toml
, then run yarn rw serve, it doesn't seem to do anything. It turns out we're hardcoding it:https://github.com/redwoodjs/redwood/blob/5a7fbbb4744ce6cb7991c81c462f6b8d2a30d182/packages/api-server/src/server.ts#L14-L17
How do we reproduce the bug?
Set
host
to a value in the api table in theredwod.toml
and runyarn rw serve api
. You should see that it's still on localhost.Are you interested in working on this?