Skip to content
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

Early draft of storing temp sites in OPFS #1838

Draft
wants to merge 9 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Oct 2, 2024

Motivation for the change, related issues

follows up on #1822

description tbd

Implementation details

Testing Instructions (or ideally a Blueprint)

@@ -39,6 +36,8 @@ export interface SiteMetadata {
id: string;
name: string;
logo?: SiteLogo;
// TODO: Don't store this in SiteMetadata.
isArchived?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using isArchived maybe we could:

  • start supporting and updating whenLastLoaded each time a site is loaded
  • continue recognizing opfs-temporary sites that have been loaded in the last 24 hours
  • delete opfs-temporary sites that have been dormant for the last 24 hours

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed some commits to do this.

Copy link
Collaborator Author

@adamziel adamziel Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is the use-case where you create a bunch of Playgrounds by clicking examples or just shopping around Blueprints, and then your entire sidebar gets clogged with maybe 20 sites.

The isArchived flag was my idea of addressing that – we could have a separate sidebar button such as "recently archived" and then you go back there to recover your site. Even better, we could archive regular OPFS sites instead of deleting them. Everything from the archive would get deleted after 24 hours.

What do you think? I'm happy to be wrong here.

@brandonpayton
Copy link
Member

brandonpayton commented Oct 3, 2024

Some things left to do:

  • Stop refreshes with no query params from creating endless, temporarily persisted sites.
  • Figure out how to select existing temp-on-OPFS sites based on app URL. Some options:
    • Map name in query API params to existing temp site
    • Be explicit with a dedicated param like temp-slug that uses slug or temp-site-id that uses the UUID. It is good to distinguish between selection of persisted sites and selection of temporary sites because we likely want to handle site-not-found scenarios differently for each:
      • Persistent sites that are not found should probably be shown a warning.
      • Temporary sites that are not found might be shown a warning but could also be automatically redirected to a fresh temp site.
  • Make new temp sites actually write WP and data to OPFS. This isn't yet happening.
  • Adjust the Save to OPFS code to simply change a site's storage type.

@brandonpayton
Copy link
Member

Added one more item to the above list:

  • Adjust the Save to OPFS code to simply change a site's storage type.

I also took a look at making sure opfs-temporary sites actually had OPFS mounts, but when I did so, WordPress installation failed because the site's directory already existed with a wp-runtime.json file in it. So we might need to adjust something either about timing or with unzipping WP.

@brandonpayton
Copy link
Member

Also changed

  • Make default temp site creation update query params. Otherwise, refreshing with no query params will continue to create new temp sites.

to

  • Stop refreshes with no query params from creating endless, temporarily persisted sites.

@bgrgicak
Copy link
Collaborator

bgrgicak commented Oct 3, 2024

I took some time and added a few things.

There is now a temp-slug that's added to every temp site.
I suggest that we rename it to site-slug later if we don't find a use for having a different name. (There is no use now.)

If a site-slug or temp-slug doesn't exist and the user already has sites, a message will be displayed.
I updated the existing message to include a Start a new Playground button.

Screenshot 2024-10-03 at 10 36 26

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 3, 2024

Oh no! Saving fresh sites to OPFS takes a considerable amount of time, like 12 seconds. Such a long waiting time is quite a bad first user experience. Remembering my explorations around zip streaming and OPFS, here's a breakdown of the things that require significant processing time:

  • Unzipping
  • Moving files through postMessage
  • Writing files using an asynchronous OPFS handle – sync handles are much faster

Perhaps we could start with a simple MEMFS site and then run a non-blocking synchronization in the background while somehow postMessage–transmitting files in bulk and not on at a time.

The risk there would be the user closing their browser tab before the sync is complete – we'd need to add an window.onunload modal and track the synchronization state to show the site is corrupted on the next visit.

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 3, 2024

Aha, but WordPress is not present in OPFS yet during the very first site boot, therefore we don't have to block loading the site for the 12 seconds it takes to synchronize MEMFS to OPFS – there are no files to synchronize yet.

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 3, 2024

I've just pushed archived sites management and the code to write new temp-opfs sites to OPFS.

We'll still need to retain the "in-memory" storage for, e.g., Safari in private mode.

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 3, 2024

The experience I went for is this:

  • Temp sites are archived after a page refresh
  • The currently open temp site isn't getting archived after a page refresh. This is flaky as I might have multiple browser tabs open – we could make it so that requesting a ?temp-slug unarchives a site.
  • Archived sites created < 24h ago are deleted

I'm having second thoughts. I wonder how will that feel for the user – sometimes a site disappears, sometimes it doesn't. Also, we'll eventually need something to prevent conflicting writes to the same OPFS site from multiple Playground instances. Alternatively, we could always run those sites in a SharedWorker and avoid any synchronization. That wouldn't work in Safari today, but maybe that's okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants