Request Handler: Serve static files if they exist in VFS. Don't guess. #702
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replaces approximate rules such as
```js
if(path.startsWith('/wp-content')) {
// serve from VFS
} else {
// serve from remote server
}
```
With actual file existence check.
When a static file is requested from Playground, the service worker
doesn't know whether to serve it from VFS or playground.wordpress.net.
It tries to guess that based on approximate rules such as
`path.startsWith('/wp-content')`. This usually works, but is not
very reliable. For example, when you store an entirely new WordPress instance
in VFS, you would expect all the static assets to be served from VFS,
but that is not what the request handler would do.
This does not play well with previewing WordPress Pull Requests as we're
actually replacing the entire WordPress instance in VFS with an
arbitrary branch.
Closes #109
1. Start Playground `npm run start`
2. Use it for a bit, go to wp-admin, confirm that static assets like CSS
and JS files are loaded without issues
3. Upload something, confirm that worked too
CC @sejas @danielbachhuber as this is a breaking change in public API and may be
relevant for wp-now. While that sounds scary, everything should "just work" without
major changes required in wp-now, other than perhaps removing usages of
the `isStaticFilePath` option.
adamziel
added
[Type] Enhancement
Makes sense, thanks for the heads up! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces approximate rules such as
With actual file existence check.
Context
When a static file is requested from Playground, the service worker doesn't know whether to serve it from VFS or playground.wordpress.net. It tries to guess that based on approximate rules such as
path.startsWith('/wp-content'). This usually works, but is not very reliable. For example, when you store an entirely new WordPress instance in VFS, you would expect all the static assets to be served from VFS, but that is not what the request handler would do.This does not play well with previewing WordPress Pull Requests as we're actually replacing the entire WordPress instance in VFS with an arbitrary branch.
Related to #700
Closes #109
Testing Instructions
npm run startBreaking change
The
isStaticFilePathoption is removed in this PR..This is a breaking change in public API and may be relevant for wp-now CC @sejas @danielbachhuber.
While that sounds scary, everything should "just work" without major changes required in wp-now, other than removing this usage of
isStaticFilePath:https://github.com/WordPress/playground-tools/blob/d792d7f03323aa0ec34e5885e4aebd5741d96f24/packages/wp-now/src/wp-now.ts#L53-L65