Place 'charset' element at the top of <head> #28119

Ryz0nd · 2021-08-14T19:20:15Z

Bug

fixes: #26534
I'm sorry. I know there are people assigned to this issue, but I wanted to solve this issue as soon as possible.

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

styfle

@Ryz0nd This looks good. Any reason why you closed it?

Ryz0nd · 2021-09-08T16:22:57Z

@styfle I just thought this issue had been forgotten. Thank you for your review!

styfle

Thanks!

timneutkens

This needs a review from @janicklas-ralph

packages/next/pages/_document.tsx

janicklas-ralph · 2021-09-08T22:31:06Z

Added a comment. We might still encounter the issue with this fix.

janicklas-ralph · 2021-09-13T22:02:55Z

I slightly misunderstood the requirement here. If we only care about <meta charset> being on top then this should work.

valoricDe · 2021-10-12T15:06:43Z

@janicklas-ralph do you know where this test error originates from
https://github.com/vercel/next.js/pull/28119/checks?check_run_id=3547125652
syntax error .button { missing closing block.
I don't think it comes from the change in here, does it?

valoricDe · 2021-10-12T15:19:39Z

So in general we have possibly defined charset meta tags in next/component head or in document head.
It seems we currently only collect and dedupe meta charsets from next/head (#27142)
If one would define charsets in both places we should probably show a warning right?

ijjk · 2022-02-06T19:26:23Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
buildDuration	15.5s	15.4s	-68ms
buildDurationCached	6s	6s	-39ms
nodeModulesSize	359 MB	359 MB	-14 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.169	3.061	-0.11
/ avg req/sec	788.79	816.7	+27.91
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.322	1.333	⚠️ +0.01
/error-in-render avg req/sec	1891.13	1875.1	⚠️ -16.03

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	27.4 kB	27.4 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.2 kB	71.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.37 kB	2.37 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	4.94 kB	4.94 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.19 kB	2.19 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.3 kB	14.3 kB	✓

Client Build Manifests

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
buildDuration	19s	18.8s	-162ms
buildDurationCached	6s	6s	⚠️ +82ms
nodeModulesSize	359 MB	359 MB	-14 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.16	3.121	-0.04
/ avg req/sec	791.23	800.92	+9.69
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.316	1.343	⚠️ +0.03
/error-in-render avg req/sec	1899.78	1861.32	⚠️ -38.46

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	27.4 kB	27.4 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.3 kB	71.3 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.36 kB	2.36 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	4.97 kB	4.97 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.21 kB	2.21 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.3 kB	14.3 kB	✓

Client Build Manifests

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	Ryz0nd/next.js charset-first	Change
index.html gzip	533 B	533 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	528 B	528 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 5502a5d

ijjk

Change looks good and as mentioned above seems to not cause any conflict, thanks for the PR!

philip-nomad · 2022-02-17T11:37:14Z

Looks good!

TheThirdRace · 2022-03-05T04:01:47Z

@Ryz0nd
Would it be possible to check this out again.

I just tried and, while the situation "improved", charset is still only the second from the top.

<head>
    <meta name="viewport" content="width=device-width">
    <meta charset="utf-8">

I don't set the charset or the viewport meta tags myself, I let NextJs create it for me.

I did try setting them following the example in the PR, but the result was a lot worst as both were way down in the <head>

Ryz0nd requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners August 14, 2021 19:20

ijjk added the type: next label Aug 14, 2021

This comment has been minimized.

Sign in to view

Ryz0nd closed this Aug 29, 2021

styfle reviewed Sep 8, 2021

View reviewed changes

Ryz0nd reopened this Sep 8, 2021

styfle previously approved these changes Sep 8, 2021

View reviewed changes

timneutkens requested changes Sep 8, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

janicklas-ralph reviewed Sep 8, 2021

View reviewed changes

packages/next/pages/_document.tsx Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Ryz0nd dismissed styfle’s stale review via c64d720 February 5, 2022 04:45

Ryz0nd force-pushed the charset-first branch from 8d42222 to c64d720 Compare February 5, 2022 04:45

Ryz0nd requested review from leerob and steven-tey as code owners February 5, 2022 04:45

ijjk added the examples Issue/PR related to examples label Feb 5, 2022

Hoist the next/head element

2d9fbf7

Ryz0nd force-pushed the charset-first branch from c64d720 to 2d9fbf7 Compare February 5, 2022 05:05

Ryz0nd requested a review from timneutkens February 5, 2022 05:09

This comment has been minimized.

Sign in to view

Merge branch 'canary' into charset-first

de37a33

This comment has been minimized.

Sign in to view

Merge branch 'canary' into charset-first

5502a5d

ijjk approved these changes Feb 6, 2022

View reviewed changes

ijjk merged commit 6090101 into vercel:canary Feb 6, 2022

natew pushed a commit to natew/next.js that referenced this pull request Feb 16, 2022

Place 'charset' element at the top of <head> (vercel#28119)

1e4de80

balazsorban44 mentioned this pull request Feb 28, 2022

next/head order changed #34885

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Place 'charset' element at the top of <head> #28119

Place 'charset' element at the top of <head> #28119

Ryz0nd commented Aug 14, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

styfle left a comment

Ryz0nd commented Sep 8, 2021

styfle left a comment

timneutkens left a comment

This comment has been minimized.

This comment has been minimized.

janicklas-ralph commented Sep 8, 2021

janicklas-ralph commented Sep 13, 2021 •

edited

valoricDe commented Oct 12, 2021

valoricDe commented Oct 12, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Feb 6, 2022

ijjk left a comment

philip-nomad commented Feb 17, 2022

TheThirdRace commented Mar 5, 2022

Place 'charset' element at the top of <head> #28119

Place 'charset' element at the top of <head> #28119

Conversation

Ryz0nd commented Aug 14, 2021 • edited

Bug

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

styfle left a comment

Choose a reason for hiding this comment

Ryz0nd commented Sep 8, 2021

styfle left a comment

Choose a reason for hiding this comment

timneutkens left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

janicklas-ralph commented Sep 8, 2021

janicklas-ralph commented Sep 13, 2021 • edited

valoricDe commented Oct 12, 2021

valoricDe commented Oct 12, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Feb 6, 2022

Stats from current PR

ijjk left a comment

Choose a reason for hiding this comment

philip-nomad commented Feb 17, 2022

TheThirdRace commented Mar 5, 2022

Ryz0nd commented Aug 14, 2021 •

edited

janicklas-ralph commented Sep 13, 2021 •

edited