-
Notifications
You must be signed in to change notification settings - Fork 240
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
Manage state through redux #1731
base: trunk
Are you sure you want to change the base?
Conversation
I've been reading over this and hope to contribute later today. (But if you happen to finish it, @adamziel, that is fine and good too :) |
}, | ||
reducers: { | ||
setPlaygroundClient: ( | ||
setActiveSite: (state, action: PayloadAction<SiteInfo>) => { | ||
state.activeSite = action.payload; |
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.
Should we check whether the site is part of the current sites list?
Yay! I'm wrapping up for now so feel free to take over |
@@ -22,7 +23,8 @@ const SITE_METADATA_FILENAME = 'playground-site-metadata.json'; | |||
* NOTE: We are using different storage terms than our query API in order | |||
* to be more explicit about storage medium in the site metadata format. | |||
*/ | |||
export type SiteStorageType = 'temporary' | 'opfs' | 'local-fs'; | |||
export const SiteStorageTypes = ['opfs', 'local-fs', 'none'] as const; |
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 temporarily changed all the storage options naming to these three, but before this PR is merged I'd love to ensure the Query API keeps working with today's storage options names.
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.
It would be great if we could block merging PRs that contain changes with some special TODO pattern like @BEFORE-MERGE
, so we could leave inline notes for ourselves and be stopped from merging if we forget to address them.
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.
Actually, we could just do this with a CI test.
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 would love that!
With this particular change, though, we may not need to do that since the storage
Query API parameter is gone. Developer documentation will need updating for sure.
wpVersion: | ||
resolvedBlueprint.preferredVersions?.wp || | ||
LatestMinifiedWordPressVersion, | ||
phpVersion: resolveVersion( |
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 if we allow site info to specify an unsupported PHP version and simply warn in the UI and tell the user how it will be handled by the runtime?
Over time, previous site configurations may have PHP versions that are no longer supported, and IMO, it would be better not to lose the information that a site was created for a now-unsupported PHP version. And if a site is now fataling due to back-compat breaks between the requested and actual PHP versions, it would give the user a chance to understand why.
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.
Great point ❤️ Let's do that.
* should it come from OPFS? Local directory? Network? We could | ||
* separate the "load from" and "save" to operations, but they | ||
* make more sense as user interactions than URL parameters. | ||
* Perhaps we only need a single `load-site-from` URL parameter? |
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.
Here are some thoughts...
It sounds like we're talking about the following three data flows:
- Import: Import site into local data store
- Use: Work with site in local data store
- Export: Export site from local data store
From Playground's perspective, all sites could be local, with manual or automated import/export operations as another layer.
As for the query API, it seems we have a couple of cases:
- Create: Create a site with a specific configuration
- Load: Identify a site to load and use its persisted configuration**
What if the query API explicitly supports distinct create and load operations?
The create operation could do things like:
- Create a temporary site with or without blueprint
- Create a site that is stored in OPFS with or without blueprint
- Create a site that is stored on the local filesystem with or without blueprint
- Create a new local site based on a one-time import
- Create a new local site that updates itself based on an import each time it loads
- Create a new local site that knows how to save (i.e., export) to an external data store
and is able to export to an external data store, either manually or automatically.
The load operation could be as simple as https://playground.wordpress.net/?load=<uuid>
, and the site configuration could be looked up based on browser storage.
The only time the storage medium needs to matter is when a site is created. When we are loading sites, we take a single identifier and look up the site configuration from whatever unified browser data store we choose.
** Perhaps some query params can be allowed to tweak an existing site's config to use a different PHP or WP version, but for security and safety, maybe doing so should be a create operation that forks to a temporary site until the user explicitly saves over the original site.
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.
It sounds like we're talking about the following three data flows:
- Import: Import site into local data store
- Use: Work with site in local data store
- Export: Export site from local data store
I agree about the Import, Use, Export scenarios. At the same time, "local data store" seems too narrow. The three local stores we support today are:
- OPFS
- Local directories
- In-memory sites
There's also a case of moving the site between local data stores, e.g. "save this temporary site" (in-memory -> OPFS) or "save on the disk" (* -> local directory).
Browsers keep evolving so we may have more in the future.
I would also like to support remote data sources like GitHub repos, remote zip files etc. These are remote, not local, and could fall under the importing data flow. I just want to acknowledge them. One use-case I have in mind is visiting, say, https://playground.wordpress.net/github/adamziel/blog, and getting a temporary site loaded from a GitHub repo.
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.
As for the query API, it seems we have a couple of cases:
- Create: Create a site with a specific configuration
- Load: Identify a site to load and use its persisted configuration**
What if the query API explicitly supports distinct create and load operations?
I like it! One nuance is we'll still need to handle running an existing site with different runtime configuration, e.g. let's turn off the networking and turn on more PHP extensions
. I'm now thinking we may remove the concept of specifying ?networking=yes
via URL for an existing site and always read that flag from the persistent store. This would mean we won't be able to change that configuration without interacting with the "site settings form", but maybe that's fine 🤔 It would be a bit less convenient for the developers, but it would save us merging a cascade of config values.
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.
Hm, I like your idea more and more @brandonpayton.
I'm thinking about the state transition between Create
and Load
and it seems non-trivial. Let's consider a few scenarios:
- Creating a new OPFS site. Once we update the data store, we need to transition to a
Load
state. This requires updating the URL to ensure a page refresh won't create another site. What should we preserve in the URL? My gut says only the site slug. The rest of it should already be in the store, e.g. the Blueprint and the runtime configuration like networking and PHP extension bundles. - Creating a new MEMFS site. When transitioning to a
Load
state we need to preserve everything in the URL to make sure a page refresh creates an identical site. I'm not even sure if we want any site slug in the URL, as we'd try to resolve that site slug after a page refresh. The site wouldn't be there anymore, so we'd fail. - Saving a MEMFS site in OPFS. This seems identical to "Creating a new OPFS site" scenario. We're just delaying the state transition until a specific user interaction.
Which means we'd get an inconsistent URL behavior based on the storage type. I'm not sure what to think about it yet.
One alternative to get a more consistent behavior would be keeping all the query args and the blueprint in the URL for OPFS sites, and only prepending a ?site-slug=opfs:my-new-site
param. On the up side, it would preserve all the other parameters. On the down side, the user may easily miss the ?site-slug
part of the URL and get frustrated when their query arg adjustments don't work. We could detect that and explain how it works in the UI, although I'm worried how effective would that be – the current GitHub modal explains you're going to lose your data and even requires checking a checkbox, and it still confuses people.
We'd also have to decide what to do in cases like https://playground.wordpress.net/site/opfs:my-site/?networking=yes
. We could either apply that on top of any stored configuration, or ignore these values. If we ignore them, an explicit warning in the UI would go a long way.
Tangentially, I like thinking about the query args as of CLI args – they're used to start the program and I can re-run the last command. Eventually we could share more names and code paths between the Playground website and Playground CLI.
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.
Actually, should we even support ?storage=browser
? It effectively says someone can create a persistent site in your store by just you clicking a link. That sounds invasive. Perhaps only supporting a temporary store via the Query API would be a better choice? And it would clear up a lot. I'm not sure how we could handle ?storage=local-fs
anyway.
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.
Actually, should we even support ?storage=browser? It effectively says someone can create a persistent site in your store by just you clicking a link. That sounds invasive.
Intuitively, this was a concern for me as well. It's a concern about a URL being able to decide too much for a user before they have a chance to consider it. It seems like we will come to a stronger sense of what is appropriate over time, and always starting with a temporary sites seems like a very safe place to start.
Removing the
storage
query arg would unlock the following flow:
- You click on a Playground link.
- You get a new temporary site. The URL stays unchanged. There's no way to create a persistent site through a link.
❤️ This sounds good and wise to me.
You can...
- Refresh the page and get a new site created with the same recipe as the original site.
- Switch to another site and get a different URL. We keep the
client
andiframe
around so you continue seeing the temporary site in the site list and can return to it without losing your changes.
Good stuff.
- Click "store this site permanently". You're asked for a name or we pick a random name for you, and then the URL changes to point to the stored site. Blueprints and things like
?plugin=
are removed from the URL.
One thing I wonder about is whether there is any security drawback to loading sites based on human-readable slugs that can be guessed (instead of just addressing persistent sites with randomly assigned UUIDs). Human-readable slugs are pleasant for humans IMO and possibly good for easy human URL recognition. Yet slugs might be more easily guessed by third parties who could craft links to them.
Whether that is a danger or not probably depends on whether a site-loading URL can trigger anything more than simply loading a site. If a URL can trigger loading and a site and running remote blueprint X, then third parties could manipulate sites by injecting things, deleting things, or whatever.
In this case, opening Playground would still default to a temporary site like it does today. The only way to get a persistent site would be by explicitly clicking the "save" button. The subsequent visits to Playground would also default to a new temporary site. If that annoys the users, we might explore defaulting to the last used permanent site.
This makes sense to me and sounds generally good.
[snip]
We also wouldn't have to figure out what?storage=
means in terms of "where do I load it from" vs "where do I save it to". I'd say let's just do it. What do you think @brandonpayton?
Let's do 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.
I tried that in 1ad41dd. The new feature flag is http://localhost:5400/website-server/?site-manager. I have about 30 minutes before I wrap up from this PR for the day, let's see where can I get it.
Awesome. I'll play with it a bit now.
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.
One nuance is we'll still need to handle running an existing site with different runtime configuration, e.g. let's turn off the networking and turn on more PHP extensions.
I wonder whether we should actually have first-class UI support for this use. It could be something like "Try with a different configuration" that prompts the user for config adjustments before launching a fork of the site.
Tangentially, I like thinking about the query args as of CLI args – they're used to start the program and I can re-run the last command. Eventually we could share more names and code paths between the Playground website and Playground CLI.
👍 This seems like a good thought. If our concepts are well-defined, it seems like this could work out pretty well.
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.
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've spent a bit more time here and wow this simplified a lot. I've reduce the amount of types, code paths, and logic. I like this! I'm wrapping up this work for the weekend, feel free to take over @brandonpayton
...ges/playground/website/src/components/ensure-playground-site/ensure-playground-site-slug.tsx
Outdated
Show resolved
Hide resolved
if (requestedSiteSlug !== 'create') { | ||
const siteInfo = await getSiteInfoBySlug(requestedSiteSlug!); | ||
console.log({ siteInfo }); | ||
dispatch(setActiveSite(siteInfo!)); |
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 if the identified site does not exist?
One scenario:
- User bookmarks Playground link for specific site slug/ID
- User deletes site, perhaps unintentionally, or clears browser storage
- User visits bookmark
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.
We need a notification system to tell the user what happened as we redirect to the first site that exists. There are WordPress components for that. It should be fine to delay that to another PR.
packages/playground/website/src/components/ensure-playground-site/index.tsx
Outdated
Show resolved
Hide resolved
@brandonpayton I've restructured the PR and, while we could make more improvements, I'm happy with the big picture architecture. I'd love to learn what do you think. Otherwise, let's wrap up all the loose ends and move towards merging. |
Planning to resume work on this in the morning. |
@@ -85,6 +85,9 @@ export function compileBlueprint( | |||
onStepCompleted = () => {}, | |||
}: CompileBlueprintOptions = {} | |||
): CompiledBlueprint { | |||
// Deep clone the blueprint to avoid mutating the input | |||
blueprint = JSON.parse(JSON.stringify(blueprint)); |
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.
Non-rhetorical question: Is there a reason we shouldn't use structuredClone()
here?
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 only reason I didn't use it was I forgot it exists, thank you :D
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'm AFK tomorrow and Monday but plan to continue reviewing and wrapping up remaining tasks as able. I left some notes as questions but can address the concerns raised.
@adamziel mentioned that we should be able to switch between temporary sites without booting them anew each time. This is important so users don't lose state, and he mentioned using a UI component to remember existing Playground instances (maybe just the temporary instances). I'm planning to look at doing that if he doesn't get to it first.
It would be lovely if we could get this merged and in a good place before WCUS next week.
@@ -23,6 +30,48 @@ body { | |||
text-decoration: none !important; | |||
} | |||
|
|||
.components-modal__header-heading-container .components-modal__header-heading { |
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.
@adamziel, where did these style choices come from? Are they derived from some sort of common definitions used to create the Figma designs for the webapp redesign?
I am asking in order to know how to find these things more directly. Before you started working on this, I was looking at specific nodes in Figma to get styles for specific component instances. Hopefully, there is a better way.
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 started with the ones proposed by Figma, but they did not look like the design. I tweaked them until the site visually match the design. It seems like the SanFrancisco webfont and the SanFrancisco apple system fonts loaded by my web browser are not quite the same.
try { | ||
return JSON.parse(rawData); | ||
} catch (e) { | ||
return JSON.parse(encodeStringAsBase64(rawData)); |
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 use decodeBase64ToString()
instead?
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 yes, good spot
Instead of adjusting the Cypress E2E tests, let’s take a detour that will delay this PR and see if we can rebuild the failing E2E tests in Playwright. And whatever Cypress tests still succeed, just let them be. This is a good opportunity to avoid locking ourselves in Cypress too much, more context: #885 (comment) |
Description
Implements a large part of the website redesign:
High-level changes shipped in this PR:
This work is a convergence of 18+ months of effort and discussions. The new UI opens relieves the users from juggling ephemeral sites and losing their work. It opens up space for long-lived site configurations and additional integrations. We could bring over all the PR previewers and demos right into the Playground app.
Here's just a few features unblocked by this PR:
Use Theme Unit Test Data as a starter content #718.
Remaining work
Unhandled Promise Rejection: UnknownError: Invalid platform file handle
when saving a temporary site to OPFS.name=""
etc. accessibility attributes we need, see AXE tools for Chrome.storage
query arg (it's removed in this PR)TODOs
added in this PR and decide whether to solve or punt themLocal Filesystem
UI in browsers that don't support them. Don't just hide them, though. Provide a help text to explain why are they disabled.updateSite
in redux-store.ts vsupdateSite
insite-storage.ts
. What would an unambiguous code pattern look like?updateSite(slug, newConfig)
in a React Component and being done without thinking "ughh I still need to update OPFS" or "I also have to adjust that .json file over there"Follow up work
packages/docs/site/docs/main/about/build.md
cc @juanmaguitar.list()
, still return that site's info but make note of the error. Not showing that site on a list could greatly confuse the user ("Hey, where did my site go?"). Let's be explicit about problems.console.js:288 TypeError: Cannot read properties of undefined (reading 'apply') at comlink.ts:314:51 at Array.reduce (<anonymous>) at callback (comlink.ts:314:29)
– it seems to happen at trunk, too.URL
,runtimeConfiguration
,Blueprint
, andsite settings form state
for both OPFS sites and in-memory sites. Let's see if we can make it reusable in Playground CLI."password"
.Closes #1719
cc @brandonpayton