-
Notifications
You must be signed in to change notification settings - Fork 322
[Blueprints] Use the local worker in Builder in development mode #2495
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
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.
Hi @mho22, thank you for working on this. This issue has bothered me for a while, but I haven't taken the time to fix it.
I left some questions and suggestions below, but this looks like a good thing for us to do.
@@ -215,6 +215,8 @@ const validRemoteOrigins = [ | |||
'https://localhost', | |||
'http://127.0.0.1', | |||
'https://127.0.0.1', | |||
'http://127.0.0.1:4400', | |||
'http://localhost:4400', |
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.
Is there any downside to using
import { remoteDevServerHost, remoteDevServerPort } from '../../build-config';
in this module and defining these string using those imports?
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.
No, you're right I should use these configs instead. On it.
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.
done.
target: | ||
mode === 'production' | ||
? 'https://playground.wordpress.net' | ||
: `http://${remoteDevServerHost}:${remoteDevServerPort}`, |
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.
IIRC, the purpose of this bit is to forward requests to playground.wordpress.net/plugin-proxy.php
when a plugin-proxy.php request is received by the the dev server. The plugin-proxy.php
script requires PHP and a GH_TOKEN environment var, so I think maybe we want to keep forwarding such requests to playground.wordpress.net where there is both a PHP server and that secret defined.
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.
done.
import.meta.env.MODE === 'production' | ||
? `https://playground.wordpress.net/plugin-proxy.php?${proxyParams}` | ||
: `http://${remoteDevServerHost}:${remoteDevServerPort}/plugin-proxy.php?${proxyParams}` |
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.
Could we conditionally define the target origin once in this module and replace these ternaries with a single template str? For example:
import.meta.env.MODE === 'production' | |
? `https://playground.wordpress.net/plugin-proxy.php?${proxyParams}` | |
: `http://${remoteDevServerHost}:${remoteDevServerPort}/plugin-proxy.php?${proxyParams}` | |
`${playgroundServerOrigin}/plugin-proxy.php?${proxyParams}` |
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.
On a related note, what if we default to the current origin when in production so both of the following could work?
https://playground.wordpress.net/builder/builder.html
https://my.favorite.self.hosted.playground.domain/builder/builder.html
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.
done. It is way more readable!
@brandonpayton Thank you for your review! I added the corrections you suggested. |
@@ -542,7 +538,7 @@ const runBlueprint = async (editor) => { | |||
const blueprintCopy = JSON.parse(blueprintString); | |||
await startPlaygroundWeb({ | |||
iframe: playgroundIframe, | |||
remoteUrl: `https://playground.wordpress.net/remote.html`, | |||
remoteUrl: 'remote.html', |
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.
@mho22, when I test this now, it just uses https://playground.wordpress.net/remote.html again. I think we might need to conditionally set a root URL at the top of this module and use throughout instead of just using relative URIs.
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.
@brandonpayton I missed that one, sorry. I managed to add a ternary operator to separate the officialRemoteOrigin
with the new devRemoteOrigin
.
If import.meta.env.MODE
is not equal to development
it will fall back to https://playground.wordpress.net
. I am not sure about the use of import.meta.env.MODE
but I think it is fine here.
e9c28f1
to
73d2708
Compare
73d2708
to
ec31db0
Compare
Hi @mho22, I tested a bit more and discovered a couple of bugs:
I pushed a commit to fix both of those issues by:
Do these changes look OK to you? If so, please feel free to merge this. |
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.
Approved if @mho22 is OK with the latest changes
@brandonpayton I have just one question. After that, we can squash and merge based on your answer. |
@mho22, I'm sorry if I am missing something obvious, but where can I find your question? |
@brandonpayton Aha! I think I messed something up. You’re seeing it now, right? |
@mho22 yep! Thanks! I made the fix and tested locally, and I'll go ahead and merge this as long as there are no build errors. |
Thank you @brandonpayton 🎉 ! |
Motivation for the change, related issues
In order to find a solution to issue #2466. I struggled to go through the
worker-thread
run on port 4400 as the one used in the builder is theplayground.wordpress.net
one when runningnpm run dev
.Implementation details
If
import.meta.env.MODE === 'production
then use the remote worker, else use the local one.Additionally, due to complex caching, other steps should be taken into account to get a fresh worker during page refresh. Explained below.
I didn't modify the following files :
And possible errors can occur in these tests :
Testing Instructions
Add a test log inside the
boot
method inworker-thread.ts
.Run
npm run dev
Go to http://127.0.0.1:5400/website-server/builder/builder.html
Unregister the http://127.0.0.1:4400 service worker.
Delete the
playground-cache-
inCacheStorage
data in theCacheStorage
section inApplication
tab from the browser developer tools.Refresh page.
You should see the log.
boot
method inworker-thread.ts
and reproduce the steps 3 to 5.You should see the modified log.
Extra steps if the service worker stops running unexpectedly :
Bypass for network
in theService Worker
section in theApplication
tab from the browser developer tools and unregister the service worker fromhttp://127.0.0.1:4400
.Bypass for network
in theService Worker
section in theApplication
tab from the browser developer tools and do a hard refresh the page again.