-
Notifications
You must be signed in to change notification settings - Fork 249
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
Backfill remote asset listing when needed #1531
Conversation
@brandonpayton could we add some tests for this PR? |
Yep! |
The boot process doesn't need to know about web-related compromises. That can be handled by the process that invokes bootWordPress().
585b9fd
to
3e6d6ea
Compare
wpVersion: string | ||
): string | undefined { | ||
return wpVersion in SupportedWordPressVersions | ||
? `wp-${wpVersion}` |
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 purpose of this is to explicitly export the knowledge of where static files are located and to avoid duplicating that knowledge elsewhere.
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.
Good call!
@bgrgicak I reworked this to remove the static asset list retrieval from boot and move it to worker-thread which is AFAIK is a better, web-specific place for it.
I'm not sure yet but may have to withdraw the "Yep!". It may be possible to check this with e2e tests, and I plan to look at that in the morning. |
I updated this PR with manual testing instructions and am considering whether we might add e2e tests for this. |
The next step after this PR is to re-add the better request routing based on the remote asset path lookup. Should be straightforward now that can count on the listing file existing when needed. |
I had a TODO to consider a But another function is unnecessary. Just treat the path and base separate using the new URL(
joinPaths(...),
schemeHostAndBasePath
) |
Something feels off about this check. Imagine we're loading a minified build and stream-backfilling all the static assets. At one point, that CSS file will be created which means it will be detected on the next load. That doesn't mean all the files are there. Perhaps checking for the presence of explicitly created |
|
||
if ( | ||
wpStaticAssetsDir !== undefined && | ||
!primaryPhp.fileExists(remoteAssetListPath) && |
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.
Shouldn't this check if the remoteAssetListPath exists (vs doesn't exist)?
!primaryPhp.fileExists(remoteAssetListPath) && | |
primaryPhp.fileExists(remoteAssetListPath) && |
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.
Oh, I see, you're checking if that file is missing from the WordPress build and if we need to fetch it. Hm. I'm not sure I follow – I thought that list is only supposed to exist in the minified builds? Official WordPress releases don't ship one.
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 think it is correct as-is because we want to retrieve the file on-demand if it does not already exist.
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 would be a case where it doesn't exist in the minified zip bundle but does exist on the server?
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.
Aha, that would be the case for builds that are already cached in the browser storage today.
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.
Oh, I see, you're checking if that file is missing from the WordPress build and if we need to fetch it. Hm. I'm not sure I follow – I thought that list is only supposed to exist in the minified builds? Official WordPress releases don't ship one.
@adamziel That's the challenge here. We don't want to retrieve this listing unless it is needed. It normally just comes with minified WP builds, but once we start depending on the existence of the listing file (#1532) and encounter an older minified WP build in browser or device storage, we need to retrieve the listing so the minified build continues to work. That was also the motivation of checking for the single wp-admin/css/common.css
file as a hint that we are potentially dealing with a minified build.
Do you have another idea about how we could or should approach this so we don't retrieve anything unnecessarily or add to the filesystem of full WP builds? Note this is the worker-thread in @wp-playground/remote
, in case that influences your thinking.
cc @bgrgicak because I think the question of detecting full WP also affects #1532.
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 is it with our timing here? 😄 I keep responding just as you respond in a way that makes my comment stale. 😂
Thanks for looking at this again today, @adamziel!
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.
Aha, that would be the case for builds that are already cached in the browser storage today.
This is actually why I reverted the routing changes the second time... because of existing browser storage without the file.
It was intended as a sort of better-than-nothing check to avoid fetching the remote asset listing when we already have a full WP install or something like that. But what we could do is this:
Pro: This would mean only retrieving the file once if needed. 🤔 Perhaps we could avoid this altogether when using device storage. |
Nevermind. Today we are just loading Playground WP builds in device storage AFAICT. To keep things simple, I'll reduce this to a simple check for listing existence, retrieve once, and write the contents without filtering. |
The questionable common-file check is now removed. |
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.
Looking good, thank you @brandonpayton!
@bgrgicak I took some time to try to add an e2e test to cover this dynamic retrieval as part of the e2e tests added in #1539, but I'm getting strange results where the listing file is asserted to exist but encounters a file-does-not exist error when trying to unlink. It would be good to cover more of this worker-thread logic with automated tests, but I'm going to move on for now. |
Motivation for the change, related issues
In order to reintroduce the request routing improvements from #1490, we need to be sure that existing browser storage can backfill the wordpress-remote-asset-paths file when needed. That is the purpose of this PR.
Implementation details
During boot, we check whether the
/wordpress/wordpress-remote-asset-paths
exists. If it does not exist, we check a static file path that should be common across WP versions,wp-admin/css/common.css
. If the common static file also does not exist, we guess we are dealing with a minified WP build cached in browser storage without a remote asset listing file, and we download it based on detected WordPress version (as long as the detected version is a supported WP version).Testing Instructions (or ideally a Blueprint)
So far, I have tested this manually with steps like the following:
/wordpress/wordpress-remote-asset-paths
exists and then delete the file/wp-<X>/wordpress-remote-asset-paths
/wordpress/wordpress-remote-asset-paths
exists again. Then delete the file./wp-<X>/wordpress-remote-asset-paths
. Regardless of the WP version in the query string, we should continue requesting the paths listing for version X because that is what is in browser storage./wordpress/wordpress-remote-asset-paths
exists again.