feat(cli): Enable `host` option for `yarn rw serve` #8385

Josh-Walker-GM · 2023-05-22T13:25:11Z

Problem
We need to have some consistent way to set the host used when running yarn rw serve [side] and we also want to default to localhost.

See #8019 (comment)

Solution
We add a --host option to the yarn rw serve command. It defaults to the value inside the appropriate redwood.toml option. These toml options themselves default to localhost when not explicitly defined.

We should also be aware that when we run both sides using yarn rw serve we preferentially choose the web config over the api config. We have this because we offer both options but only start one fastify server when running both sides.

…ocalhost'

replay-io · 2023-05-22T13:36:22Z

19 replays were recorded for d1e1fba.

0 Failed

19 Passed

requireAuth graphql checks

useAuth hook, auth redirects checks

locator.waitFor: Target closed
=========================== logs ===========================
waiting for locator('text=Username `testuser@bazinga.com` already in use') to be visible
============================================================

Check that a specific blog post is prerendered
Check that about is prerendered
Check that homepage is prerendered
Check that meta-tags are rendering the correct dynamic data
Check that rehydration works for page not wrapped in Set
Check that rehydration works for page with Cell in Set
Check that rehydration works for page with code split chunks
Check that you can navigate from home page to specific blog post
Waterfall prerendering (nested cells)
RBAC: Admin user should be able to delete contacts

RBAC: Should not be able to delete contact as non-admin user

locator.waitFor: Target closed
=========================== logs ===========================
waiting for locator('text=Username `admin@bazinga.com` already in use') to be visible
============================================================

Smoke test with dev server
Smoke test with rw serve
Loads Cell mocks when Cell is nested in another story
Loads Cell Stories
Loads MDX Stories
Mocks current user, and updates UI while dev server is running

View test run on Replay ↗︎

jtoar · 2023-05-22T19:37:31Z

Just to confirm that this fix will work for you, @dennemark are you running the api-server independently of the CLI? We should probably make this change anyway just to be consistent, but if you are, the work here won't solve the bug for you, which I think you could've only really experienced by running the api-server binary as-is and not through the CLI

Update: the api-server CLI handlers are also for yarn rw dev

won't make much of a difference for getPaths but getConfig isn't memoized so that one is a little excessive to keep calling

jtoar

LGTM @Josh-Walker-GM, thanks!

jtoar

Probably okay'ed a bit too early; I should test a few more things because there's a lot of entry points here

yarn rw serve
yarn rw dev
running api-server's bins (rw-api-server-watch, rw-server) independently of the CLI
- it's unlikely that rw-api-server-watch is used independently of the CLI
passing flags via CLI
setting config in redwood.toml

`yarn rw dev`


Flag	n/a
Toml	`api` settings are respected

Now if you set host in the api table in redwood.toml:

 [api]
   port = 8911
+  host = "0.0.0.0" # normally 'localhost'

yarn rw dev doesn't work out of the box. Before it did, but only because host was ignored, which could also be considered a bug.

To get it working again, you have to set RWJS_DEV_API_URL (an env var that has no TOML-equivalent nor is it used anywhere else) to "http://0.0.0.0" so that the webpack dev server proxy works again:

redwood/packages/core/config/webpack.development.js

Line 19 in 0de84e4

target: `${process.env.RWJS_DEV_API_URL ?? 'http://localhost'}:${port}`,

Again, I'm not saying this is a bug or is undesirable. It's just different than it was before.

jtoar · 2023-05-23T00:06:50Z

Here's an old one this is related to: #6362

packages/cli/src/commands/serve.js

dennemark · 2023-05-23T06:16:29Z

Just to confirm that this fix will work for you, @dennemark are you running the api-server independently of the CLI? We should probably make this change anyway just to be consistent, but if you are, the work here won't solve the bug for you, which I think you could've only really experienced by running the api-server binary as-is and not through the CLI

Update: the api-server CLI handlers are also for yarn rw dev

So much progress while I was asleep... thanks so much!

Within my development environment I do not need @redwoodjs/api-server. But when I try to run my docker container, I need to install @redwoodjs/api-server and @redwoodjs/internals seperately. (I am not fully sure why, anymore, since those packages should be installed anyways). But when starting the docker container, I am calling

yarn rw-server api
(follows roughly this docker version: https://github.com/redwoodjs/docker/blob/a983aca3b95bdccfb339cf1f6dfcb6e5bbde22b5/docker/jeliasson-nginx/api/Dockerfile#L90)

Btw. right now I have set host on api and web side in redwood.toml. Both stating 0.0.0.0. But I only added the host to api side yesterday, hoping it would resolve my docker issue (i downgraded api-server for now). I think I would prefer clear seperation of api and web host. So on api side api || localhost and on web side web || localhost, since I would not assume web host to affect api side.

packages/core/config/webpack.development.js

jtoar · 2023-05-24T02:17:38Z

@dennemark I think I've got my head wrapped around all the changes here now. Here's the state of things:

With this PR, if you run the web and api servers separately, the settings for each will be respected. But if you run them together, since we only start one fastify server at the moment, the web settings will be used.

You said you were running yarn rw-server api, so the settings in your [api] TOML table should be respected now.

I'll merge this and release an RC; if you could upgrade to it (I'll follow up with the version once it's published) and confirm that it works the way you expect that'd be greatly appreciated!

jtoar

Thanks @Josh-Walker-GM, and thanks for letting me hijack this one.

I want to follow up on a few more discussions with others, but want to get an RC out for testing.

* Add 'host' option which defaults to toml value - which defaults to 'localhost' * Update api-server cli-handlers too * style: save project config calls in var won't make much of a difference for getPaths but getConfig isn't memoized so that one is a little excessive to keep calling * style: save project-config calls where possible * style: prefer ?? to || for undefined values * style changes * enable api-server to take flag * remove stray nullish accessor * style: remove needless instantiation * use existing config in scope * style * set host in dev handler * remove needless null checks * style: make api command more readable * add host to watch * get dev test passing * get serve test passing * c * get watch working * remove console log * comments * style * fix dev handler when web host is set * revert to expected webpack behavior * fix test --------- Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>

jtoar · 2023-05-24T05:29:25Z

@dennemark I just released this PR in an RC: 5.2.3-rc.3+ddbd1f431. You can upgrade to it via yarn rw upgrade -t rc. The hash after the + may not be there, but it shouldn't matter. Would love to know if this fixes it for you

This reverts commit e831517.

jtoar · 2023-05-24T16:51:41Z

@dennemark another update: I've changed my by plan here. I'm going to revert #8019 and release a patch till we've got this sorted out in main and know all the edge cases. So this PR won't be available in the patch when I actually release it. Sorry for the churn! I'll follow up one more time when v5.2.3 is released

dennemark · 2023-05-24T18:03:06Z

@dennemark another update: I've changed my by plan here. I'm going to revert #8019 and release a patch till we've got this sorted out in main and know all the edge cases. So this PR won't be available in the patch when I actually release it. Sorry for the churn! I'll follow up one more time when v5.2.3 is released

Thanks so much for the effort!

So i am going to test it in one of the follow up releases?

Going to check the commits tomorrow. Curious, why this issue is quite complex :)

jtoar · 2023-05-24T18:22:28Z

@dennemark yeah you can ignore the RC I mentioned earlier and use this patch instead https://github.com/redwoodjs/redwood/releases/tag/v5.2.3.

This patch just reverts #8019, so you don't have to set api.host in redwood.toml. We still want to make it so that you can configure api.host in redwood.toml, but it was easier and faster to revert the breaking change we made.

The reason for complexity here is 1) there's a lot of different deploy environments and 2) there's a lot of different combinations to these settings among the CLI commands, and checking them all just takes time. (And I have other things I'm supposed to be working on instead, so I kind of need this one wrapped for the time being.)

dennemark · 2023-05-24T18:56:10Z

Thanks for the heads up and your effort :)

This reverts commit 26ad3e2.

* Revert "Reserve h for help, and format docs (#8407)" This reverts commit 70b7c4d. * Revert "feat(cli): Enable `host` option for `yarn rw serve` (#8385)" This reverts commit 26ad3e2. * Revert "Fastify server: Default to localhost (#8019)" This reverts commit d609db6.

Add 'host' option which defaults to toml value - which defaults to 'l…

1661437

…ocalhost'

Josh-Walker-GM requested review from Tobbe and jtoar May 22, 2023 13:25

Josh-Walker-GM changed the title ~~Add 'host' option which defaults to toml value - which defaults to 'l…~~ feat(cli): Enable host option for yarn rw serve May 22, 2023

Josh-Walker-GM added the release:feature This PR introduces a new feature label May 22, 2023

Josh-Walker-GM and others added 5 commits May 22, 2023 21:11

Update api-server cli-handlers too

d72be40

style: save project config calls in var

7f75daa

won't make much of a difference for getPaths but getConfig isn't memoized so that one is a little excessive to keep calling

style: save project-config calls where possible

02bdf36

style: prefer ?? to || for undefined values

a518ce8

Merge branch 'main' into jgmw-serve-host

dd01b40

jtoar approved these changes May 22, 2023

View reviewed changes

jtoar added this to the next-release-patch milestone May 22, 2023

jtoar suggested changes May 23, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

jtoar reviewed May 23, 2023

View reviewed changes

packages/cli/src/commands/serve.js Outdated Show resolved Hide resolved

jtoar added 9 commits May 23, 2023 13:43

style changes

b7235b2

enable api-server to take flag

6f9bdc0

remove stray nullish accessor

20f2555

style: remove needless instantiation

7f1f94e

use existing config in scope

6d1856e

style

aebc6e6

set host in dev handler

f2e70ec

remove needless null checks

cad21b3

style: make api command more readable

43405a5

jtoar reviewed May 23, 2023

View reviewed changes

packages/core/config/webpack.development.js Outdated Show resolved Hide resolved

add host to watch

8477e0a

jtoar added 3 commits May 23, 2023 18:05

Merge branch 'main' into jgmw-serve-host

8aafdee

revert to expected webpack behavior

c2fc19d

fix test

d1e1fba

jtoar approved these changes May 24, 2023

View reviewed changes

jtoar merged commit 26ad3e2 into main May 24, 2023

jtoar deleted the jgmw-serve-host branch May 24, 2023 02:37

jtoar modified the milestones: next-release-patch, v5.2.3 May 24, 2023

Tobbe mentioned this pull request May 24, 2023

Reserve h for help, and format docs #8407

Merged

jtoar added a commit that referenced this pull request May 24, 2023

Revert "feat(cli): Enable host option for yarn rw serve (#8385)"

5f71b08

This reverts commit e831517.

jtoar modified the milestones: v5.2.3, next-release May 24, 2023

jtoar modified the milestones: next-release, v6.0.0 May 25, 2023

jtoar added a commit that referenced this pull request Jun 28, 2023

Revert "feat(cli): Enable host option for yarn rw serve (#8385)"

17065c9

This reverts commit 26ad3e2.

jtoar added a commit that referenced this pull request Jun 28, 2023

Revert "feat(cli): Enable host option for yarn rw serve (#8385)"

fddc9a2

This reverts commit 26ad3e2.

jtoar mentioned this pull request Jun 28, 2023

chore(v6): temporarily revert host-related changes #8765

Merged

jtoar modified the milestones: v6.0.0, v7.0.0 Jun 28, 2023

jtoar modified the milestones: SSR, v7.0.0 Dec 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): Enable `host` option for `yarn rw serve` #8385

feat(cli): Enable `host` option for `yarn rw serve` #8385

Josh-Walker-GM commented May 22, 2023 •

edited

Loading

replay-io bot commented May 22, 2023 •

edited

Loading

jtoar commented May 22, 2023 •

edited

Loading

jtoar left a comment

jtoar left a comment •

edited

Loading

This comment was marked as duplicate.

jtoar commented May 23, 2023

dennemark commented May 23, 2023 •

edited

Loading

jtoar commented May 24, 2023

jtoar left a comment

jtoar commented May 24, 2023 •

edited

Loading

jtoar commented May 24, 2023

dennemark commented May 24, 2023

jtoar commented May 24, 2023

dennemark commented May 24, 2023

feat(cli): Enable host option for yarn rw serve #8385

feat(cli): Enable host option for yarn rw serve #8385

Conversation

Josh-Walker-GM commented May 22, 2023 • edited Loading

replay-io bot commented May 22, 2023 • edited Loading

jtoar commented May 22, 2023 • edited Loading

jtoar left a comment

Choose a reason for hiding this comment

jtoar left a comment • edited Loading

Choose a reason for hiding this comment

yarn rw dev

This comment was marked as duplicate.

jtoar commented May 23, 2023

dennemark commented May 23, 2023 • edited Loading

jtoar commented May 24, 2023

jtoar left a comment

Choose a reason for hiding this comment

jtoar commented May 24, 2023 • edited Loading

jtoar commented May 24, 2023

dennemark commented May 24, 2023

jtoar commented May 24, 2023

dennemark commented May 24, 2023

feat(cli): Enable `host` option for `yarn rw serve` #8385

feat(cli): Enable `host` option for `yarn rw serve` #8385

Josh-Walker-GM commented May 22, 2023 •

edited

Loading

replay-io bot commented May 22, 2023 •

edited

Loading

jtoar commented May 22, 2023 •

edited

Loading

jtoar left a comment •

edited

Loading

`yarn rw dev`

dennemark commented May 23, 2023 •

edited

Loading

jtoar commented May 24, 2023 •

edited

Loading