-
Notifications
You must be signed in to change notification settings - Fork 327
Playground CLI: Support --auto-mount=path option #2525
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
Conversation
type: 'boolean', | ||
default: false, | ||
describe: `Automatically mount the specified directory. If no path is provided, mount the current working directory. You can mount a WordPress directory, a plugin directory, a theme directory, a wp-content directory, or any directory containing PHP and HTML files.`, | ||
type: 'string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The auto
in the name now seems to kind of lose its semantics:
- The
--auto-mount
variant without the path looks fine because it "automatically mounts the CWD". - The
--auto-mount=path
makes me wonder what the difference is from--mount=path
. In other words, what's the "auto" for, and why do we need that in addition to--mount=path
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe the "auto" means that it automatically resolves the mount target? Sorry, I don't know the semantics of these arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the meaning of auto
is to resemble what wp-now does, i.e. if it's a plugin directory, mount it in wp-content/plugins
, if it's a theme, in wp-content/themes
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I wonder, given @JanJakes point, whether we could also achieve the same using a environment variable and thus not modifying the semantics? For example:
|
@akirk Now that I understand the difference between
Do you see any advantages to using |
My concern is that we're too complex in options but I guess this is already the case before this modification. Now that I think more about it, this solution makes a lot of sense if you wanted to automount multiple directories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this change!
@akirk and @JanJakes thanks for all your feedback.
We don't currently support auto-mounting multiple directories, but depending on which directories folks want to auto-mount, we could consider implementing that in the future. |
Motivation for the change, related issues
It is impossible (or at least awkward) to test the Playground CLI
--auto-mount
option locally using thenpx nx unbuilt-jspi playground-cli
family of commands. The reason is that--auto-mount
tries to auto mount the current working directory, but those commands must be executed with the Playground source directory.For this case, it would be useful to be able to pass a path to the
--auto-mount
option.In addition, it is more powerful and flexible for to regular users to be able to pass an explicit
--auto-mount=path
.Implementation details
This PR:
--auto-mount
option to a string.--auto-mount
yargs description.runCLI()
to default auto-mount to the current working directory if--auto-mount
is specific but no path is provided.Testing Instructions (or ideally a Blueprint)