Skip to content

bootPlaygroundRemote: Defer setting the iframe src initially, to prevent flash of unstyled content #519

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 3 commits into from
Jun 22, 2023

Conversation

eliot-akira
Copy link
Collaborator

@eliot-akira eliot-akira commented Jun 3, 2023

What?

Removes a line in bootPlaygroundRemote that initially sets iframe src to route /.

This pull request defers setting the src attribute, letting the client set it when the website is ready, e.g., afer blueprint steps are run and desired theme activated.

Possibly this could be an opt-in behavior.

Why?

Prevents flash of unstyled content, for example, when a default theme is shown before switching to a newly installed theme.

How?

Should check if this doesn't break something unexpected. The PR will require all callers of bootPlaygroundRemote to set the iframe src themselves. Otherwise it will stay empty.

Testing Instructions

  1. Check out the branch.
  2. Run npm run dev.
  3. Visit a URL with a different theme activated, and confirm that the default theme does not show for a moment before switching to the active theme.

…h of unstyled content

This allows the src attribute to be set later when client is ready, e.g., afer blueprint steps are run and desired theme activated
@adamziel
Copy link
Collaborator

adamziel commented Jun 5, 2023

Thinking about this more, what does it to to a website? Does it still loading the homepage in that iframe? AFAIR it should because it uses a Blueprint with a landingPage which always loads a page in the end, but just wanted to confirm. All the more reasons to use Blueprints. Would you also document this change in the place where you'd expect to find such info? The doc site lives in packages/docs/site and can be built with, AFAIR, nx dev docs-site

@adamziel
Copy link
Collaborator

adamziel commented Jun 19, 2023

@eliot-akira I've been noodling on this for a while now and I'm quite confident this is fine. The only side-effect is that opening remote.html directly displays a white page since there's no client to set the initial iframe URL. For that reason, it would be nice to add a query parameter to configure this, e.g. remote.html?no-initial-url or something to that effect. Then, calling startPlaygroundWeb() would always set that parameter.

@dmsnell
Copy link
Member

dmsnell commented Jun 19, 2023

remote.html?no-initial-url

Preferably something semantically related to the goal. ?all-at-once or ?on-ready or something else.

Another question is whether this (option) is necessary at all. Is there a reason not to wait until the Playground is ready to route the iframe?

@eliot-akira
Copy link
Collaborator Author

eliot-akira commented Jun 19, 2023

The only side-effect is that opening remote.html directly displays a white page since there's no client to set the initial iframe URL.

I wonder if we could consider remote.html as an internal implementation detail instead of a public interface/contract - so that returning an empty page is expected behavior? And the client can be expected to set the initial iframe src when ready, i.e., after blueprint steps are completed.


remote.html?no-initial-url or ?all-at-once or ?on-ready

How about a new optional query parameter url for the initial route?

That would make it an opt-in behavior to set the iframe src initially (before blueprint steps are run). If not set, then by default bootPlaygroundRemote() will not set the iframe src (as per this PR) to let the client decide when to set it.


To confirm that it does this currently, I looked in the playground client's startPlaygroundWeb() function.

It calls runBlueprintSteps() (CompiledBlueprint.run()), which calls playground.goTo() after the steps are done, with the path being the blueprint option landingPage or / by default. That sets the iframe src to the internal scoped URL.

This goTo() gets called even if there are no blueprint steps to run. However, I see at least one possible way that it does not called, which is when a blueprint step throws an error. In that case, the iframe will be empty - unless we move this call:

await (playground as any).goTo(
	blueprint.landingPage || '/'
);

..to be inside the try/catch finally clause. Then it would set the initial iframe src even if a blueprint step fails.

(EDIT: That seems like a good idea in any case, I made a commit 881f508 to move it.)

… so that it runs even when a blueprint step throws
@eliot-akira eliot-akira changed the title bootPlaygroundRemote: Don't set iframe src initially, to prevent flash of unstyled content bootPlaygroundRemote: Defer iframe src initially, to prevent flash of unstyled content Jun 19, 2023
@eliot-akira eliot-akira changed the title bootPlaygroundRemote: Defer iframe src initially, to prevent flash of unstyled content bootPlaygroundRemote: Defer setting the iframe src initially, to prevent flash of unstyled content Jun 19, 2023
@adamziel adamziel added [Type] Enhancement New feature or request Public API labels Jun 22, 2023
@adamziel adamziel marked this pull request as ready for review June 22, 2023 16:23
@adamziel
Copy link
Collaborator

adamziel commented Jun 22, 2023

Oki I'm on board and I think this makes sense. I don't even think we need the URL parameter. This is even better because the blank page makes it apparent there's something wrong with using remote.html without the API client. The } finally { adjustment is great. Thank you @eliot-akira!

I wonder if we could consider remote.html as an internal implementation detail instead of a public interface/contract - so

You have to explicitly embed it to use it, though, but yeah I no longer think it needs to render anything by default.

@adamziel adamziel merged commit 679681b into WordPress:trunk Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants