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

WIP: Use selected site settings #1707

Closed
wants to merge 2 commits into from

Conversation

brandonpayton
Copy link
Member

Motivation for the change, related issues

The settings for the selected site are not properly applied when switching between sites with the site switcher. This PR is meant to fix that.

Implementation details

TBD

Testing Instructions (or ideally a Blueprint)

TBD

@brandonpayton brandonpayton added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] Website [Feature] Boot Flow labels Aug 27, 2024
@brandonpayton brandonpayton self-assigned this Aug 27, 2024
@brandonpayton
Copy link
Member Author

I am currently working through boot issues with this. The initial site boots, but selecting another site leads to a "Playground already booted" error.

Planning to resume looking at this in the morning.

This is a fragile change but should be only temporary
as we try tighten up the site storage data model.
@brandonpayton
Copy link
Member Author

The already-booted message seems to have been caused by useBootPlayground() getting blueprint and mountDescriptor at different times and causing startPlaygroundWeb() to be called multiple times for the same configuration.

This is pretty messy stuff and will be until we finishing building out the models for temporary, browser-stored, and device-stored through redux.


collectWindowErrors(logger);

const query = new URL(document.location.href).searchParams;
const blueprint = await resolveBlueprint();
Copy link
Member Author

Choose a reason for hiding this comment

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

To support switching between persisted sites, we can no longer just resolve the blueprint once in module scope but rather need to do it within the component every time the selected site changes or the site settings change.

Copy link
Collaborator

@adamziel adamziel Aug 29, 2024

Choose a reason for hiding this comment

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

Blueprint, as in site setup steps, should only run once when a site is created. To load the site we only need the same runtime configuration, e.g. the PHP version.

The problem seems to be calling startPlaygroundWeb() twice in a row while targeting the same remote.html frame. We might need to rethink this flow entirely, e.g. create a new remote.html iframe instance for each booted site.

* @param siteInfo The site info to convert to Playground query params
* @returns URL with query params representing the site info
*/
export function siteInfoToUrl(baseUrl: URL, siteInfo: SiteInfo): URL {
Copy link
Collaborator

@adamziel adamziel Aug 29, 2024

Choose a reason for hiding this comment

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

Aha! We've hit the runtime configuration vs site configuration problem (see "Runtime setup options"). This is an important one. Let's not rush a solution but take it slow and methodically. We're effectively building a v1 of the runtime configuration interface here.

Let's treat things like languages, themes, plugins, multisite etc. as build-time features that are only relevant when the site is first created. If we tried re-applying them to an existing site, we'd end up with, say, two copies of the same theme – that's not what the user expects.

That leaves us with settings that don't trigger any filesystem– or database–level updates in /wordpress. For sure that's networking, PHP version, and landing page. Probably also login, even though it causes some database writes. What else?

We don't yet have the plumbing to hotswap the WordPress version so let's remove the "WP Version" from the "edit site" form for now. Same for the language and the storage option. That's tangential but relevant.

Here's another question: When user changes the runtime configuration of an existing site, how will we save that information? We can either alter the original Blueprint, or store runtime configuration in another file. What would you do here?

Tangentially related: Down the road we will provide a way of applying a Blueprint to an existing site. At that time, we will want to combine the new Blueprint with the original one used to boot that site. There's nothing immediately actionable today. Regardless of the direction this PR takes, I'm just noticing the problem of rewriting/merging Blueprints will come back on more than one occasion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder what would be the ripple effect of decoupling the runtime options across JS API, Blueprints API, Query API, if any.

For example, imagine a version of the Query API that distinguishes between "site build options" and "runtime options":

https://playground.wordpress.net/?build[plugin]=gutenberg&runtime[networking]=yes

These long names would be difficult to type and to remember so our current short parameters (?plugin=gutenberg&networking=yes) seem to be a more useful choice.

However, Blueprints and JS API are quite verbose already so maybe there may be an opportunity for an API refresh in there. If not, perhaps the documentation could distinguish between the two types of configuration options 🤔

Copy link
Collaborator

@adamziel adamziel Aug 29, 2024

Choose a reason for hiding this comment

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

Also, notice this translates Blueprint->Query args only for Playground to do Query args->Blueprint. What if we passed the Blueprint directly? Or even just the site slug, since we know the config for each slug?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be highly useful to have stable site URLs. Say I go to playground.wordpress.net/my–base–theme and it always brings up the same site. Also, the site itself wouldn't use a random numeric scope, but the slug

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we could pair that with github autocommit and offer public site urls, e.g. public.playground.wordpress.net/adamziel/blog

Copy link
Collaborator

Choose a reason for hiding this comment

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

And that could later form v1 of collaboration and site sync. Cc @akirk @griffbrad @dawidurbanski @dmsnell

@adamziel
Copy link
Collaborator

adamziel commented Sep 6, 2024

I think #1731 supersedes this one so I'll go ahead and close.

@adamziel adamziel closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Boot Flow [Package][@wp-playground] Website [Type] Bug An existing feature does not function as intended
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants