Fetch scripts in the <head> on client-side navigations
#164
Conversation
src/runtime/router.js
Outdated
| @@ -105,6 +126,11 @@ export const init = async () => { | |||
| const body = toVdom(document.body); | |||
| hydrate(body, rootFragment); | |||
|
|
|||
| // Cache the scripts. Has to be called before fetching the head. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Now that I think about it, we should probably do the same for the styles 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
But for scripts, we can only add the "new" ones:
src/runtime/router.js
Outdated
| const scriptEl = document.createElement('script'); | ||
| scriptEl.textContent = script; | ||
| return scriptEl; |
There was a problem hiding this comment.
This doesn't execute the code until you add it to the DOM, right?
There was a problem hiding this comment.
Yes, that's right. Why do you ask? 🙂
There was a problem hiding this comment.
Out of curiosity. I wasn't 100% sure 😄
BTW, we would need to copy the type="module" here because those scripts are different.
There was a problem hiding this comment.
Absolutely 🙁
There was a problem hiding this comment.
Done 0152fde 🙂
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
|
Weird, the e2e tests seem to pass just fine locally:
|
|
Weird thing number 2: Locally there are 69 tests running, but in github actions there are 90 tests: Possibly related to the number of workers? Locally, the tests run in 5 workers, whereas in CI, it's 1 worker. |
|
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. |
|
I've been triaging the issue and I think we had two problems:
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
|
I believe we need this as well. Right now, in the Movies demo for example, the |
|
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:
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? |
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
|
I'm going to merge this now 🙁 Great work, Mario! My only concern is that I'd like us not to block when calling I've also realized that we can simplify the code a little bit in the future because |
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
deferandasyncattributes 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: