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

[Bug?]: --fwd doesn't seem to be forwarding anything to dev-server #9209

Open
1 task done
getify opened this issue Sep 19, 2023 · 6 comments
Open
1 task done

[Bug?]: --fwd doesn't seem to be forwarding anything to dev-server #9209

getify opened this issue Sep 19, 2023 · 6 comments
Assignees
Labels
bug/confirmed We have confirmed this is a bug help wanted

Comments

@getify
Copy link

getify commented Sep 19, 2023

What's not working?

I consulted the docs here: https://redwoodjs.com/docs/cli-commands#dev

They mention using the CLI flag --fwd=" ... " to send along flags that control the underlying dev server (webpack I guess?).

In particular, I wanted to:

  1. stop it from opening a new web browser
  2. bind a different port
  3. bind to ALL hosts so I can hit the dev server from my phone (to do remote debugging)

I issued this command:

yarn rw dev web --fwd="--no-open --port=9234 --allowed-hosts=all"

Also tried this variant:

yarn rw dev web --fwd="--no-open --port 9234 --allowed-hosts all"

(spaces instead of =)

None of the 3 flags seem to affect anything, as the browser still opens, the port is still 8910, and I still can't hit the dev-server instance from my phone using my laptop's local LAN IP.


SIDE NOTE: the docs linked above still say to use --open=false for stopping the browser from opening. I tried that, first. But then I found another issue saying that the underlying webpack arguments had changed to --no-open. Docs should be updated there.

To be clear, though, the webpack docs do still say that --allowed-hosts all should be working, but it doesn't seem to be, as I don't think any of those params are getting forwarded (as I asserted above).

How do we reproduce the bug?

Try the above command(s) as I mentioned.

What's your environment? (If it applies)

System:
    OS: Linux 5.15 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    Shell: 5.1.4 - /bin/bash
  Binaries:
    Node: 19.9.0 - /tmp/xfs-c4e108ca/node
    Yarn: 3.6.1 - /tmp/xfs-c4e108ca/yarn
  npmPackages:
    @redwoodjs/core: 6.2.1 => 6.2.1

Are you interested in working on this?

  • I'm interested in working on this
@getify getify added the bug/needs-info More information is needed for reproduction label Sep 19, 2023
@jtoar
Copy link
Contributor

jtoar commented Sep 19, 2023

Hey @getify thanks for reporting! Yeah our docs could use more clarification, especially since we swapped the default dev server from webpack to Vite in v6.

If you just created a Redwood app (looks like it was recent since the version is v6.2.1), then you're using Vite. You'd have to opt back into webpack via the redwood.toml file:

# redwood.toml

[web]
  # how to opt back into webpack
  bundler = "webpack"

While the --fwd logic for webpack seems to work (going off the testing I just did), it does look like it's broken for Vite. I'm guessing it's the logic here which does look incorrect:

// Tries to maintain the same options as vite's dev cli
// See here: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/cli.ts#L103
// e.g. yarn rw dev web --fwd="--force"
const {
force: forceOptimize,
forwardedServerArgs,
debug,
} = yargsParser(process.argv.slice(2), {
boolean: ['https', 'open', 'strictPort', 'force', 'cors', 'debug'],
number: ['port'],
})

(Perhaps it was meant to be _: forwardedServerArgs?)

If you're interested in trying to fix the issue, here are the relevant files:

And I'm happy to provide more pointers and ultimately review. And also happy to just implement the fix of course—let me know how you'd like to proceed!

Lastly if you'd like to just get things working, you can edit these settings directly in web/vite.config.ts.

@getify
Copy link
Author

getify commented Sep 20, 2023

I'd definitely like to work on a patch, will take a look.

@getify
Copy link
Author

getify commented Sep 20, 2023

Here's what I've figured out so far:

  • This line in rw-vite-dev expects a parameter name (camel-case aliased) of --forwarded-server-args with the server options in them. On this line, the contents of that value (if any) are set as the server options to createServer(..).

    It seems like this is relying on those server options having come in as JSON-like object in the flag, for yargsParser to parse, like --forwarded-server-args="{ port: 1234, ... }"... is that accurate?

  • However, as far as I can tell from this line, the contents of the yarn rw dev --fwd=" .. " parameter have just been dumped as direct flags to the rw-vite-dev.mjs CLI, not wrapped up in a single object value passed as the --forwarded-server-args=" .. " flag.

If my analysis is correct, and there's no other magic going on here, then this seems like a fundamental mismatch between the two tools, where one (likely rw-vite-dev.mjs) will need to change for this to work.

Moreover, the options to Vite's createServer(..) indicate the relevant three configs we need to set are:

  • port -- just a number, easy

  • host -- to support passing a magic 'all' value, we'll need to manually set this property to '0.0.0.0' or to true

  • open -- to suppress opening the browser, this needs to be set to false, which is like the OLD webpack --open=false form but does not match the updated --no-open parameter form that webpack had moved to

    actually, I guess yargsParser automatically parses --no-open as open: false

Ultimately, there's some judgement calls to be made design wise here...

  1. Should we keep supporting this "forward server args" kind of design, with selective magical overriding ('all' to '0.0.0.0')?

    Moreover, should we be passing them in the object form to be re-parsed by yargsParser, or should we be decomposing these settings into single-value params?

  2. Or should we just create dedicated flags on yarn rw dev for the three in question (my OP), like --port=1234 --no-open --host=all?

@jtoar
Copy link
Contributor

jtoar commented Sep 21, 2023

If my analysis is correct, and there's no other magic going on here, then this seems like a fundamental mismatch between the two tools, where one (likely rw-vite-dev.mjs) will need to change for this to work.

@getify great analysis! Yeah there's no other magic going on here. I think this was just left in a half-finished state. And agreed that rw-vite-dev is the one of the two that needs to change.

Just to add more context, when the dev server defaulted to webpack, what yarn rw dev ultimately did for the web side under the hood was call the webpack CLI's serve command, which is the webpack-dev-server package. (A lot of indirection here.) That CLI has many options, so the --fwd flag made sense. (Here's the PR that added it: #904. It was added to support configuring the port actually.)

I think, in terms of getting this fixed faster at least, implementing the --fwd flag for rw-vite-dev is the way to go. Adding dedicated --port and --host flags makes us have to answer more questions like does that mean the web port and host or the api port and host? And the only issue with more questions is the earliest I could get more of the team to think about this is probably after the conference (so in ~2 weeks). But thoughts?

@getify
Copy link
Author

getify commented Sep 21, 2023

implementing the --fwd flag for rw-vite-dev is the way to go.

OK, I think I understand your intention here. But need a bit more clarification.

So... let's say someone ran: yarn rw dev --fwd='--whatever=42'. That would end up invoking rw-vite-dev.mjs --whatever=42, right?

In other words, rw-vite-dev needs to parse out not just one "fwd" flag, but potentially a half dozen server-oriented flags. According to their docs, here are the potential options someone might want to pass to the underlying createServer(..) call:

  • host
  • port
  • strictPort
  • https
  • open
  • proxy
  • cors
  • headers
  • hmr
  • watch
  • middlewareMode
  • fs.strict, fs.allow, fs.deny
  • origin
  • sourcemapIgnoreList

So first question: which of these (or all?) do we want to support? Is there any reasonable justification for only supporting forwarding some of these?

Next question: are we going to create individual flags (in rw-vite-dev) for the ones we support? Like --fwd='--port=1234 --host=all'?

If we're doing individual flags, should we follow some naming convention for these individual flags, like --server-host, --server-headers, --server-fs-allow, etc? That way it's clear in the docs (for rw-vite-dev) that this set of flags are all specifically controlling the createServer(..) call.

Or if we don't want to do individual flags (perhaps because that's too many?), should we instead have folks provide a single JSON-like object, at a param like --server-args, like this --fwd='--server-args={port:1234,...}'?

@jtoar
Copy link
Contributor

jtoar commented Sep 21, 2023

So first question: which of these (or all?) do we want to support? Is there any reasonable justification for only supporting forwarding some of these?

For sure; I should emphasize that the main driver behind my design decisions at the moment is how to fix this as fast as possible for you while not locking us out of a proper long-term solution. I think the long-term solution is swapping out rw-vite-dev for Vite's actual CLI. But I'd have to spend some time researching why we didn't do that in the first place, so a near-term fix (this week basically) isn't likely.

If you're not concerned with getting a fix this week, then I can stop thinking that way and suggest that we think about swapping out rw-vite-dev for Vite's actual CLI.

But to keep answering your questions with my current line of thinking, the justification for only supporting forwarding some flags is 1) I only have anecdotal data, but those three flags (open, port, host) are by far the most common. And 2) if we support forwarding them all, we should really just do the right thing and swap out rw-vite-dev for Vite's actual CLI.

Answering your next question, yeah rw-vite-dev would have individual --open, --port, and --host flags. Going off how yargsParser is configured, it seems like this was started:

} = yargsParser(process.argv.slice(2), {
boolean: ['https', 'open', 'strictPort', 'force', 'cors', 'debug'],
number: ['port'],
})

I.e., yargsParser is set up to parse an open flag. I feel like there should've been a transformation to process.argv to inline the contents of the --fwd flag prior to passing it to yargsParser. And the properties like open, etc, should've been destructured, not forwardedServerArgs.

Lastly I'd push back against the --server- prefix just because it would make our Vite CLI start to diverge from Vite's actual CLI more than it already has, and I just don't have enough good reasons to do that yet.

And sorry this is somewhat confusing haha, I didn't do the work here so I can only make guesses at intention. So appreciate you bringing this up and discussing it with me!

Josh-Walker-GM pushed a commit that referenced this issue Dec 5, 2023
Adds 2 debug configurations to vscode/launch.json

- Web Debugger (launches the built-in chrome web debugger)
- Compound of Dev + Api + Web (launches a fully debuggable redwood with
a single configuration)

It makes sense to disable the browser open in the redwood.toml if you
want to use the web debugger alone or in compound.

```
[browser]
  open = false
 ```
 
 It'd also be possible to add a --fwd="--open=false" but that is currently being discussed as an issue in #9209

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
jtoar pushed a commit that referenced this issue Dec 5, 2023
Adds 2 debug configurations to vscode/launch.json

- Web Debugger (launches the built-in chrome web debugger)
- Compound of Dev + Api + Web (launches a fully debuggable redwood with
a single configuration)

It makes sense to disable the browser open in the redwood.toml if you
want to use the web debugger alone or in compound.

```
[browser]
  open = false
 ```
 
 It'd also be possible to add a --fwd="--open=false" but that is currently being discussed as an issue in #9209

---------

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
colbywhite added a commit to colbywhite/rw-form-generator that referenced this issue Dec 15, 2023
this turns it off for everyone, not just codesandbox.
but the cli doesn't currently properly forward vite props (redwoodjs/redwood#9209)
  at the moment
@jtoar jtoar added help wanted bug/confirmed We have confirmed this is a bug and removed bug/needs-info More information is needed for reproduction labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug help wanted
Projects
None yet
Development

No branches or pull requests

2 participants