Skip to content

Route requests for missing static files using remote asset metadata - Second attempt #1490

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

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

brandonpayton
Copy link
Member

Motivation for the change, related issues

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing file
to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

Implementation details

This PR:

  • Includes a list of remote asset paths with minified WP builds
  • Updates request handling to only handle missing files as static when
    they are explicitly listed as remote assets
    (perhaps this should be split into multiple PRs)
  • Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

Testing Instructions (or ideally a Blueprint)

  • CI tests
  • Test manually with npm run dev and observe that Playground loads
    normally with no unexpected errors in the console

@brandonpayton
Copy link
Member Author

brandonpayton commented Jun 6, 2024

Note: The e2e tests for this PR may fail until I push rebuilt versions of the minified WP zips.

brandonpayton added a commit that referenced this pull request Jun 6, 2024
## Motivation for the change, related issues

Remove WP 6.1 from list of supported versions because our builds only
update current WP version minus 3.

Given `WP 6.5 - 3 = WP 6.2`, this means a breaking change like #1490
will not work on the stale WP 6.1 build on playground.wordpress.net.

So it is time to drop WP 6.1.

## Testing Instructions (or ideally a Blueprint)

- CI Tests
…1417)

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

Related to #1365 in that we need better information to judge whether to
handle a request for a missing static file as PHP.

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate the request to PHP.

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which can be delegated to PHP.

- CI tests
- Examine a minified WP zip and the contents of its
wordpress-remote-asset-paths file
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
@brandonpayton
Copy link
Member Author

We are getting the following e2e failures which I am able to reproduce locally:

  2 failing                                                                                               
  1) Remote Assets                                                                                        
       should load remote assets for WordPress-6.3 and storage=browser:                             
     TypeError: Cannot set property message of [object DOMException] which has only a getter        
      at modifyErrMsg (http://localhost/__cypress/runner/cypress_runner.js:75124:15)                
      at $Cy.retry (http://localhost/__cypress/runner/cypress_runner.js:144185:29)                        
      at onFailFn (http://localhost/__cypress/runner/cypress_runner.js:143757:21)                         
      at tryCatcher (http://localhost/__cypress/runner/cypress_runner.js:1807:23)                   
      at Promise._settlePromiseFromHandler (http://localhost/__cypress/runner/cypress_runner.js:1519:31)
      at Promise._settlePromise (http://localhost/__cypress/runner/cypress_runner.js:1576:18)       
      at Promise._settlePromise0 (http://localhost/__cypress/runner/cypress_runner.js:1621:10)      
      at Promise._settlePromises (http://localhost/__cypress/runner/cypress_runner.js:1697:18)      
      at _drainQueueStep (http://localhost/__cypress/runner/cypress_runner.js:2407:12)              
      at _drainQueue (http://localhost/__cypress/runner/cypress_runner.js:2400:9)                   
      at Async._drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2416:5)            
      at Async.drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2286:14)            
                                                                                                          
  2) Remote Assets                                                                                        
       should load remote assets for WordPress-6.2 and storage=browser:                             
     TypeError: Cannot set property message of [object DOMException] which has only a getter        
      at modifyErrMsg (http://localhost/__cypress/runner/cypress_runner.js:75124:15)                
      at $Cy.retry (http://localhost/__cypress/runner/cypress_runner.js:144185:29)                        
      at onFailFn (http://localhost/__cypress/runner/cypress_runner.js:143757:21)                         
      at tryCatcher (http://localhost/__cypress/runner/cypress_runner.js:1807:23)                   
      at Promise._settlePromiseFromHandler (http://localhost/__cypress/runner/cypress_runner.js:1519:31)
      at Promise._settlePromise (http://localhost/__cypress/runner/cypress_runner.js:1576:18)       
      at Promise._settlePromise0 (http://localhost/__cypress/runner/cypress_runner.js:1621:10)      
      at Promise._settlePromises (http://localhost/__cypress/runner/cypress_runner.js:1697:18)      
      at _drainQueueStep (http://localhost/__cypress/runner/cypress_runner.js:2407:12)              
      at _drainQueue (http://localhost/__cypress/runner/cypress_runner.js:2400:9)                   
      at Async._drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2416:5)            
      at Async.drainQueues (http://localhost/__cypress/runner/cypress_runner.js:2286:14)    

I'm not sure why this is only affecting WP 6.2 and 6.3 and why it is only erroring for storage=browser. In practice, both WP versions load and reload fine with storage=browser in my local environment.

@brandonpayton
Copy link
Member Author

We are getting the following e2e failures which I am able to reproduce locally:

In local testing, the e2e failures didn't appear to have anything to do with WP 6.2 or 6.3 specifically. It seemed to have more to do with the quantity of tests, though I am not 100% on that.t also might have to do with not automatically clearing browser storage when the WP version changes in the URL.

For now, I reduced the tests to just testing storage=none and storage=browser for the default WP version.

@brandonpayton brandonpayton merged commit 7549565 into trunk Jun 7, 2024
5 checks passed
@brandonpayton brandonpayton deleted the reintroduce-proper-routing branch June 7, 2024 00:15
@brandonpayton
Copy link
Member Author

This is getting painful. I'm going to revert this because people with existing browser storage are going to have issues if /wordpress/wordpress-remote-asset-paths is not found. That was clear when this was originally reverted, but I focused on stopping the deletion of the listing file rather than considering browser storage prior to the existence of the listing.

brandonpayton added a commit that referenced this pull request Jun 7, 2024
…tadata - Second attempt" (#1492)

Reverts #1490

I'm going to revert this because people with existing browser storage
are going to have issues if /wordpress/wordpress-remote-asset-paths is
not found. That was clear when this was originally reverted, but I
focused on stopping the deletion of the listing file rather than
considering browser storage prior to the existence of the listing.

Tomorrow, when returning with a clearer mind, I plan to consider how we
might deal with this scenario.

cc @WordPress/playground-maintainers
brandonpayton added a commit that referenced this pull request Jun 10, 2024
## Motivation for the change, related issues

This PR adds a remote asset listing to minified WP builds so we can
later tell which files are expected to exist remotely and which should
be considered missing if they are not present locally.

This was originally part of request routing PRs #1417 and #1490, but
since there are some sensitive edge cases around the routing changes and
browser storage, I am breaking the build changes into their own PR so
the more sensitive changes can be reviewed more easily on their own.

## Implementation details

This PR updates the minified WP build process to generate a
wordpress-remote-asset-paths file containing the WP-relative paths of
all assets not included in the minified build. That way, we have the
necessary information to judge whether a requested resource can be
requested from playground.wordpress.net when it does not exist locally.

We include this listing with new minified WP builds.

In case browser storage has already cached a minified WP build without
this listing, we also make it available remotely so it can be retrieved
as needed.

## Testing Instructions (or ideally a Blueprint)

- CI tests
brandonpayton added a commit that referenced this pull request Jun 24, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this pull request Jun 25, 2024
## Motivation for the change, related issues

In order to reintroduce the request routing improvements in #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:
1. Install the Chrome extension [OPFS
Explorer](https://chromewebstore.google.com/detail/opfs-explorer/acndjpgkpaclldomagafnognkcgjignd?hl=en)
2. Switch Playground to browser storage and note the current WP release
_X_ in the query string
3. Open dev tools
4. Use OPFS explorer to confirm
`/wordpress/wordpress-remote-asset-paths` exists and then delete the
file
5. Switch to the dev tools Network tab
6. Reload the page
7. Filter the requests and confirm there was a request for the current
WP release _X_ like `/wp-<X>/wordpress-remote-asset-paths`
8. Clear the Network request log
9. Use OPFS explorer to confirm
`/wordpress/wordpress-remote-asset-paths` exists again. Then delete the
file.
10. Manually adjust the query string to point to request a different WP
release.
11. Reload the page
12. Return to the dev tools Network tab, filter the requests, and
confirm there was a request for the current WP release _X_ like
`/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.
13. Review the console log and confirm there is a warning like "Loaded
WordPress version (X) differs from requested version (Y)" and confirm
that _Y_ matches the version requested via the query string.
14. Use OPFS explorer to confirm
`/wordpress/wordpress-remote-asset-paths` exists again.
brandonpayton added a commit that referenced this pull request Jun 27, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
brandonpayton added a commit that referenced this pull request Jul 13, 2024
… Second attempt (#1490)

Prior to this PR, we could not easily tell whether we should request a
missing static asset from the web or delegate a request for a missing
file to WordPress.

Related to #1365 where we need better information to judge whether to
handle a request for a missing static file as PHP.

This PR:
- Includes a list of remote asset paths with minified WP builds
- Updates request handling to only handle missing files as static when
they are explicitly listed as remote assets
(perhaps this should be split into multiple PRs)
- Updates request routing to behave more like a regular web server

By including a list of remote asset paths with minified WP builds, we
can judge which missing static file paths represent remote assets and
which are truly missing and need to be delegated to WordPress.

- CI tests
- Test manually with `npm run dev` and observe that Playground loads
normally with no unexpected errors in the console
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant