Fetch scripts in the `<head>` on client-side navigations by michalczaplinski · Pull Request #164 · WordPress/block-interactivity-experiments

michalczaplinski · 2023-02-28T01:28:19Z

Fixes #153

This PR is a very crude first attempt to fetch the scripts on client-side navigations. It does not work with inline scripts, violates the defer and async attributes and probably breaks heaps of other things.

This functionality is however needed in order for the WP Movies Demo to showcase it's full capability so we should include (even a half-working) solution:

For example, in this movies demo, if we land on the homepage and navigate to a movie page, https://github.com/c4rl0sbr4v0/wp-movies-demo/pull/19#issuecomment-1436547876 (a block only present in the movies pages) won't work because the actions needed haven't been defined.

luisherranz

I think it's ok for a temporary fix 🙂

Right now it only works for the head, so maybe we should pass document to the fetchHead function and rename it to fetchAssets or something. It can still return the head, though.

const fetchHead = async (head) => {

luisherranz · 2023-02-28T16:09:10Z

src/runtime/router.js

 		const body = toVdom(document.body);
 		hydrate(body, rootFragment);

+		// Cache the scripts. Has to be called before fetching the head.


Why is that?

I guess "has to" is a strong word. It should say "should be called".

It's because if we don't cache the scripts that are present on the initial page load, then they will be fetched again inside of the fetchHead() because we call fetchHead() on the initial page load as well.

Now that I think about it, we should probably do the same for the styles 🙂

Take into account that styles and scripts are not equal. Styles need to be added each time you navigate to a page because they may have been removed, but scripts only need to be executed once, even if they exist on multiple pages.

You're right, but in this case, I think it's okay. I'm only talking about a minor optimization: Add the stylesheets to the cache on the initial page load so that we don't have to download them again when we navigate to a new page.

As you said, we still have to add both the stylesheets currently present in the head and the "new" ones:

https://github.com/WordPress/block-hydration-experiments/blob/1d67b24047f4c7cd7692adc3468e642298a8acbe/src/runtime/router.js#L67-L68

But for scripts, we can only add the "new" ones:

https://github.com/WordPress/block-hydration-experiments/blob/1d67b24047f4c7cd7692adc3468e642298a8acbe/src/runtime/router.js#L65

src/runtime/router.js

luisherranz · 2023-02-28T16:11:52Z

src/runtime/router.js

+		const scriptEl = document.createElement('script');
+		scriptEl.textContent = script;
+		return scriptEl;


This doesn't execute the code until you add it to the DOM, right?

Yes, that's right. Why do you ask? 🙂

Out of curiosity. I wasn't 100% sure 😄

BTW, we would need to copy the type="module" here because those scripts are different.

Absolutely 🙁

Done 0152fde 🙂

Co-authored-by: Luis Herranz <luisherranz@gmail.com>

michalczaplinski · 2023-03-17T01:11:54Z

Weird, the e2e tests seem to pass just fine locally:

michalczaplinski · 2023-03-17T01:14:23Z

Weird thing number 2: Locally there are 69 tests running, but in github actions there are 90 tests:

https://github.com/WordPress/block-interactivity-experiments/actions/runs/4346748522/jobs/7800093352?pr=164#step:7:15

Possibly related to the number of workers? Locally, the tests run in 5 workers, whereas in CI, it's 1 worker.

SantosGuillamot · 2023-03-17T08:11:17Z

I have just merged the main branch into this one. Now, I can see all the tests (96) locally, and the ones failing also fail locally.

SantosGuillamot · 2023-03-17T09:04:39Z

I've been triaging the issue and I think we had two problems:

The fetchScriptOrStyle function wasn't async and we were using Promise.all(...).map.
We weren't passing head to the function, although we are using it.

I made a commit with what I considered a fix. Tests are passing now. Feel free to revert or modify it.

Apart from that, I've realized that we don't have e2e tests for this PR, nor for client-side navigation. I believe we should create some tests covering that:

The content gets updated after client-side navigation.
Styles are applied.
New scripts work.

SantosGuillamot · 2023-03-17T10:25:54Z

Right now it only works for the head, so maybe we should pass document to the fetchHead function and rename it to fetchAssets or something. It can still return the head, though.

I believe we need this as well. Right now, in the Movies demo for example, the view.js files are included in the <body> because they are needed after the runtime.

SantosGuillamot · 2023-03-17T13:04:25Z

I've created initial tests to cover client-side navigation in this pull request.

EDIT: From what I can see there, the new scripts seem to work in the e2e tests when:

We change head for document as @luisherranz suggested.
We move the script to the head.

However, I am not able to make it work with the Movies demo.

michalczaplinski · 2023-03-21T17:46:32Z

However, I am not able to make it work with the Movies demo.

What do you mean by that? In what way did it not work?

SantosGuillamot · 2023-03-22T08:44:30Z

Right now it only works for the head, so maybe we should pass document to the fetchHead function and rename it to fetchAssets or something. It can still return the head, though.

I made this commit that aims to fetch the scripts in the whole document and not only the head.

It seems that this passes the client-side navigation tests, and I've checked it in the Movies demo, and it seems to work fine as well. Please take a look at the code because I am not sure the changes I made are 100% correct.

Create e2e tests for client-side navigation

michalczaplinski · 2023-03-23T01:10:03Z

I'm going to merge this now 🙁 Great work, Mario!

My only concern is that I'd like us not to block when calling fetchAssets() in the future. Right now we always await it, for example inside of the init() or when calling fetchPage() but we should be able to delay awaiting until the new head is retrieved from the pages cache, right before navigation to a new page happens.

I've also realized that we can simplify the code a little bit in the future because querySelectorAll() can match a group of elements like: querySelectorAll('link[rel=stylesheet], script[type=module], script[src]').

Load the scripts in the head

5d80746

luisherranz reviewed Feb 28, 2023

View reviewed changes

luisherranz mentioned this pull request Feb 28, 2023

Tracking issue: WordPress Directives Plugin 🎨 #80

Closed

23 tasks

michalczaplinski and others added 2 commits February 28, 2023 13:10

Update src/runtime/router.js

1d67b24

Co-authored-by: Luis Herranz <luisherranz@gmail.com>

Treat the module scripts differently

0152fde

michalczaplinski marked this pull request as ready for review March 17, 2023 01:09

Merge branch 'main-wp-directives-plugin' into fetch-scripts

c2a31c4

Make async and pass head to fetchScriptOrStyle

c2283be

SantosGuillamot added 4 commits March 17, 2023 13:44

Bundle css files in e2e tests

fba57bf

Change e2e structure

120693a

Add e2e tests for CSN

b7c29f3

Add test for fetching the scripts after CSN

5e8d4f7

SantosGuillamot mentioned this pull request Mar 17, 2023

Create e2e tests for client-side navigation #185

Merged

SantosGuillamot and others added 5 commits March 21, 2023 14:10

Merge branch 'main-wp-directives-plugin' into e2e-csn

06dcc6b

Add test to check styles are removed after csn

9b90b11

Update directives format

f5b4bcd

Add test to check that csn is working

32ad7df

format tests with prettier

53af1a6

SantosGuillamot added 4 commits March 22, 2023 09:22

Fix data-wp-on.click formatting

78d5f39

Merge branch 'main-wp-directives-plugin' into fetch-scripts

e6576e9

Fetch scripts and styles in whole document

377cb1d

Merge branch 'fetch-scripts' into e2e-csn

f05695e

Merge pull request #185 from WordPress/e2e-csn

118bd8e

Create e2e tests for client-side navigation

michalczaplinski merged commit ebd60ec into main-wp-directives-plugin Mar 23, 2023

luisherranz deleted the fetch-scripts branch March 23, 2023 10:24

Conversation

michalczaplinski commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luisherranz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalczaplinski commented Mar 17, 2023

Uh oh!

michalczaplinski commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SantosGuillamot commented Mar 17, 2023

Uh oh!

SantosGuillamot commented Mar 17, 2023

Uh oh!

SantosGuillamot commented Mar 17, 2023

Uh oh!

SantosGuillamot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalczaplinski commented Mar 21, 2023

Uh oh!

SantosGuillamot commented Mar 22, 2023

Uh oh!

michalczaplinski commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michalczaplinski commented Feb 28, 2023 •

edited

Loading

luisherranz left a comment •

edited

Loading

michalczaplinski commented Mar 17, 2023 •

edited

Loading

SantosGuillamot commented Mar 17, 2023 •

edited

Loading

michalczaplinski commented Mar 23, 2023 •

edited

Loading