-
Notifications
You must be signed in to change notification settings - Fork 30
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
STU-22: Add custom local domain support for WordPress sites #994
base: trunk
Are you sure you want to change the base?
Conversation
4f49875
to
57a013b
Compare
src/lib/proxy-server.ts
Outdated
try { | ||
console.log( 'Attempting to start proxy server with elevated privileges' ); | ||
|
||
if ( currentPlatform === 'win32' ) { |
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.
Apparently in Windows, we need to run the proxy with admin rights. The AI proposed using netsh
to forward to a custom port. I need someone to help me test Windows.
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.
This seems to work relatively well in my light testing, which is great 👍 Still, this is a big feature, and we will need to test it thoroughly before landing.
A couple of initial high-level thoughts:
- There are several visual issues in the
Add site
modal right now. We should get some designer input from @matt-west or someone else on how to implement this. - Users should be able to edit the domain after a site is added. This is already a big PR, so we don't necessarily need to implement that here, but it should go into the same release as this feature.
useCustomDomain, | ||
setUseCustomDomain, | ||
customDomain, | ||
setCustomDomain, | ||
customDomainError, |
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.
This is not the fault of this PR, but we should really move this state inside SiteForm
and pass it back to the parent in the onSubmit
handler. Too much prop drilling now with no clear benefit.
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.
Entirely agree, I noticed it too :)
port, | ||
phpVersion, | ||
customDomain, | ||
useCustomDomain, |
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.
Do we really need to store a boolean for useCustomDomain
in the user config? Is it not enough to store customDomain
?
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 hesitant, I believe that we can get away with just "customDomain" but a boolean gives a sense of clarity. We absolutely need both as state values in the form.
src/lib/hosts-file.ts
Outdated
if ( domainRegex.test( hostsContent ) ) { | ||
return; // Domain already exists | ||
} |
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.
What do we do when this happens..? Display some kind of error in the app?
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.
To clarify, I was thinking of cases when the domain is pointing to a different IP. Edge case, perhaps, but it ties into other error handling questions like #994 (comment)
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 basic happy path works fine on Mac. I left some comments related to small issues and different cases.
src/lib/hosts-file.ts
Outdated
const tempPath = '/tmp/wp-studio-hosts'; | ||
await writeFile( tempPath, content ); | ||
// @ts-expect-error promisify doesn't seem typed properly. | ||
await sudoExec( `cat ${ tempPath } > ${ hostsPath }`, { |
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.
It asks for permission each time I add a new site. Is there a way to save it somehow, at least for the current Studio session?
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.
If I'm reading this properly https://www.npmjs.com/package/sudo-prompt#concurrency it's probably not possible.
That said, I don't think it's a problem, I think it was more visible when we had the bug where the hosts file was modified on each "start". But right now, it's only when you start the server for the first time for a given site. So a real user is very unlikely to face the issue on a regular basis.
I pushed a revert, so we keep the first approach. Decisions, not options. :) |
Thanks @mcsf I tried adding e2e test myself for the feature, there's a good and a bad news:
Overall, I think the PR is a good state, I would love more testing specifically. cc @wojtekn @fredrikekelund |
(This somewhat breaks the layout. Feel free to revert.)
dfd8a89
to
12ce9e9
Compare
Related issues
Summary …
Implementation details
Test plan
Pre-merge Checklist
Important note This PR has been created for a huge part using AI. My main purpose with this PR was to learn both Studio's code base and learn AI while implementing a useful feature.