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

fix(dev): avoid FOUC when swapping out link tag (fix #7973) #8495

Merged
merged 3 commits into from Jun 9, 2022
Merged

fix(dev): avoid FOUC when swapping out link tag (fix #7973) #8495

merged 3 commits into from Jun 9, 2022

Conversation

timacdonald
Copy link
Contributor

@timacdonald timacdonald commented Jun 8, 2022

Description

Assuming we have a CSS file at the following path...

/resources/css/app.css

When adding a link tag to the DOM manually (i.e. via Server Side Rendering)...

<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css">

Vite will now watch this file and hot reload changes in the browser for this asset (very sweet!). When we update app.css Vite will update the link tag's href attribute like so...

- <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
+ <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">

This is great, but for a split moment there is a flash of unstyled content as the asset has not loaded yet. The browser unloads the styles from the original stylesheet and waits until the new stylesheet has loaded and been parsed before it will show the new stylings.

This PR addresses this problem. Now the process is as follows..

First the client adds a second (cloned) link tag to the DOM, leaving the original in place and still loaded by the browser...

<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
+ <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">

Once the new stylesheet has loaded, the browser will remove the original stylesheet from the DOM, leaving the new loaded stylesheet in the DOM.

- <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">

This process then continues for future updates to the CSS file.

Additional context

I have no idea where to start with adding a unit test for this. I'm a dumb back-end developer. Send help! (I've read the guides - I just don't know how to actually write the test itself for something like this, sorry!).

I added a test. I have no idea if it is good or not!


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me.

packages/playground/hmr/hmr.ts Outdated Show resolved Hide resolved
@sapphi-red sapphi-red changed the base branch from v2 to main June 8, 2022 06:47
@sapphi-red sapphi-red changed the base branch from main to v2 June 8, 2022 06:47
@sapphi-red
Copy link
Member

Would you rebase this to the main branch?

@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 8, 2022
@timacdonald
Copy link
Contributor Author

@sapphi-red thank you so much for the review. I am happy to rebase against main, but is there any chance we could look at getting this into v2. This addresses an issue with FOUC for the official Laravel plugin. We've just mentioned this in the #ecosystem chat in the Vite discord as well.

@patak-dev
Copy link
Member

Thanks for the PR, this is a big DX improvement! I think we should merge it and try it first against main, and we can then backport it to v2

@timacdonald
Copy link
Contributor Author

Sounds good. I'll get onto this tomorrow. Finishing up on my end. Thanks for checking it out!

@timacdonald timacdonald changed the base branch from v2 to main June 9, 2022 02:11
Assuming we have a CSS file at the following path...

```
/resources/css/app.css
```

When adding a link tag to the DOM manually (i.e. via Server Side Rendering)...

```html
<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css">
```

Vite will now watch this file and hot reload changes in the browser for this
asset. When we update `app.css` Vite will update the link tag's `href`
attribute like so...

```diff
- <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
+ <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">
```

This is great, but for a split moment there is a flash of unstyled content as the asset has not loaded yet. The browser unloads the styles from the original stylesheet and waits until the new stylesheet has loaded and been parsed before it will show the new stylings.

This PR addresses this problem. Now the process is as follows..

First the client adds a second (cloned) link tag to the DOM, leaving the original in place and still loaded by the browser...

```diff
<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
+ <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">
```

Once the new stylesheet has loaded, the browser will remove the original stylesheet from the DOM, leaving the new _loaded_ stylesheet in the DOM.

```diff
- <link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?">
<link rel="stylesheet" href="{DEV_SERVER_URL}/resources/css/app.css?t={TIMESTAMP}">
```

This process then continues for future updates to the CSS file.

I have no idea where to start adding a unit test for this. I'm a dumb back-end developer. Send help!

Edit: i've added a test. Not sure if it is any good though!
@timacdonald
Copy link
Contributor Author

@sapphi-red @patak-dev Rebased against main as requested 👍

@sapphi-red
Copy link
Member

Thanks!

playground/hmr/hmr.ts Outdated Show resolved Hide resolved
sapphi-red
sapphi-red previously approved these changes Jun 9, 2022
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

LTGM 🚀

Co-authored-by: 翠 / green <green@sapphi.red>
@patak-dev patak-dev changed the title fix(dev): fix FOUC when swapping out link tag (fix #7973) fix(dev): avoid FOUC when swapping out link tag (fix #7973) Jun 9, 2022
@patak-dev patak-dev merged commit 0e5c009 into vitejs:main Jun 9, 2022
@patak-dev
Copy link
Member

We backported this PR to the v2 branch. Is #6649 needed in 2.9 for your needs? An issue with that PR is that it may break integrations that aren't expecting .css files to appear in the manifest. I think it is safer to avoid backporting #6649. If only this PR is enough, we could release 2.9.11 with it.
And about #6649, two options:

  • Base the plugin work on v3 (we shouldn't be far from the first beta release, after wich things should be more stable)
  • Maybe a temporal plugin could be added that replicates the manifest including feat(dev): added assets to manifest #6649 so you can already start using the same logic and format with the v2 branch? Then for v3 you remove it.

@jessarcher
Copy link
Contributor

jessarcher commented Jun 9, 2022

@patak-dev Thanks! This PR was the main blocker for us so we've very grateful for it being backported to 2.9. Do you have an ETA on 2.9.11? We're ready to launch once that's in.

#6649 would be nice in 2.9, but we can work around it by augmenting the manifest from our plugin.

@timacdonald
Copy link
Contributor Author

Really appreciate your feedback and review on this folks ❤️ and thank you for putting your energy into Vite - we love it.

@patak-dev
Copy link
Member

@jessarcher released vite@2.9.11, good luck with the launch 🚀

Let's keep working on #6649 and merge that one in v3. The next steps would be to get you in vite-ecosystem-ci, so we can check that future releases of Vite won't break you before it happens. You only need to PR a test file here, like this one we already have for the community maintained plugin: https://github.com/vitejs/vite-ecosystem-ci/blob/main/tests/laravel.ts, and we will start getting reports on Vite Land #ecosystem-ci channel. Right now all the efforts of vite-ecosystem-ci are placed on releasing v3, if you have breaking changes you can create a vite-3 branch and use a branch field to point to it.

@timacdonald back to you! We really appreciate you working closely with us and upstreaming as much as possible so everyone benefits. Vite has grown a lot over the last year because everyone in the ecosystem using it got involved and push it forward. I'm very happy to see Laravel officially embracing Vite as an option, I think it will help both ecosystems moving forward.

@patak-dev patak-dev mentioned this pull request Jun 10, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants