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.

Sign up for GitHub

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

Jump to bottom

Fix PerformanceObserver usage for older browsers and CI #30387

Merged
merged 2 commits into from Oct 27, 2021
Merged

Fix PerformanceObserver usage for older browsers and CI #30387

merged 2 commits into from Oct 27, 2021

Conversation

styfle
Copy link
Member

@styfle styfle commented Oct 27, 2021 •
edited

@styfle styfle changed the title Fix PerformanceObserver usage for older browsers Fix PerformanceObserver usage for older browsers and CI Oct 27, 2021
@styfle styfle mentioned this pull request Oct 27, 2021
Closed
@ijjk
Copy link
Member

ijjk commented Oct 27, 2021

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
buildDuration 20.1s 19.8s -273ms
buildDurationCached 4.1s 4s -79ms
nodeModulesSize 207 MB 207 MB ⚠️ +53 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
/ failed reqs 0 0 ✓
/ total time (seconds) 3.335 3.321 -0.01
/ avg req/sec 749.58 752.74 +3.16
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.568 1.602 ⚠️ +0.03
/error-in-render avg req/sec 1594.04 1560.28 ⚠️ -33.76
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
450.HASH.js gzip 179 B 179 B ✓
framework-HASH.js gzip 42.2 kB 42.2 kB ✓
main-HASH.js gzip 28 kB 28 kB ✓
webpack-HASH.js gzip 1.45 kB 1.45 kB ✓
Overall change 71.9 kB 71.9 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
polyfills-a4..dd70.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
_app-HASH.js gzip 1.23 kB 1.23 kB ✓
_error-HASH.js gzip 194 B 194 B ✓
amp-HASH.js gzip 312 B 312 B ✓
css-HASH.js gzip 327 B 327 B ✓
dynamic-HASH.js gzip 2.38 kB 2.38 kB ✓
head-HASH.js gzip 350 B 350 B ✓
hooks-HASH.js gzip 635 B 635 B ✓
image-HASH.js gzip 4.44 kB 4.44 kB ✓
index-HASH.js gzip 263 B 263 B ✓
link-HASH.js gzip 1.87 kB 1.87 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 ✓
334f979574ae..6f4.css gzip 106 B 106 B ✓
Overall change 13.1 kB 13.1 kB ✓
Client Build Manifests
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
_buildManifest.js gzip 459 B 459 B ✓
Overall change 459 B 459 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
index.html gzip 533 B 533 B ✓
link.html gzip 546 B 546 B ✓
withRouter.html gzip 527 B 527 B ✓
Overall change 1.61 kB 1.61 kB ✓
Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
buildDuration 16.7s 17s ⚠️ +370ms
buildDurationCached 4s 4.2s ⚠️ +201ms
nodeModulesSize 207 MB 207 MB ⚠️ +53 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
/ failed reqs 0 0 ✓
/ total time (seconds) 3.326 3.425 ⚠️ +0.1
/ avg req/sec 751.75 730.02 ⚠️ -21.73
/error-in-render failed reqs 0 0 ✓
/error-in-render total time (seconds) 1.526 1.567 ⚠️ +0.04
/error-in-render avg req/sec 1638.64 1595.17 ⚠️ -43.47
Client Bundles (main, webpack, commons)
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
450.HASH.js gzip 179 B 179 B ✓
framework-HASH.js gzip 42.2 kB 42.2 kB ✓
main-HASH.js gzip 28.2 kB 28.2 kB ✓
webpack-HASH.js gzip 1.43 kB 1.43 kB ✓
Overall change 72 kB 72 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
polyfills-a4..dd70.js gzip 31 kB 31 kB ✓
Overall change 31 kB 31 kB ✓
Client Pages
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
_app-HASH.js gzip 1.22 kB 1.22 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.38 kB 2.38 kB ✓
head-HASH.js gzip 342 B 342 B ✓
hooks-HASH.js gzip 621 B 621 B ✓
image-HASH.js gzip 4.46 kB 4.46 kB ✓
index-HASH.js gzip 256 B 256 B ✓
link-HASH.js gzip 1.9 kB 1.9 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 ✓
334f979574ae..6f4.css gzip 106 B 106 B ✓
Overall change 13.1 kB 13.1 kB ✓
Client Build Manifests
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
_buildManifest.js gzip 459 B 459 B ✓
Overall change 459 B 459 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js fix-browsers-without-PerformanceObserver Change
index.html gzip 533 B 533 B ✓
link.html gzip 545 B 545 B ✓
withRouter.html gzip 528 B 528 B ✓
Overall change 1.61 kB 1.61 kB ✓
Commit: a4bbabb

@kodiakhq kodiakhq bot merged commit 3bdf80d into canary Oct 27, 2021
@kodiakhq kodiakhq bot deleted the fix-browsers-without-PerformanceObserver branch October 27, 2021 00:37
kara
kara reviewed Nov 10, 2021
View reviewed changes
docs/basic-features/image-optimization.md
The LCP element is typically the largest image or text block visible within the viewport of the page. You can verify which element this is by running the following code in the console of your page and looking at the latest result:

```javascript
new PerformanceObserver((entryList) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than deleting this helper entirely, it might make sense if we clarify that it will only work with newer browsers? I still think it would be a helpful tool for folks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kara We don't need to document PerformanceObserver usage because Next.js will call PerformanceObserver automatically. See packages/next/client/image.tsx

Maybe I misunderstood your comment though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's useful even with the warning. The warning tells you that you did something wrong after the fact. The docs should tell you how to get it right when you first try it out.

Currently the developer journey is:

  • Look at docs for next/image
  • You may be confused because you don't understand how to find the LCP image
  • Try out the next/image component without priority (because you're not clear on how to use it)
  • Immediately get warned you're doing something wrong, even though you tried to follow the docs
  • Fix the priority attribute

If we add detail to the docs, we can help devs write it correctly the first time.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we add detail to the docs, we can help devs write it correctly the first time.

I don't think it will be correct the first time because they'll have to write the code, then copy/paste a script, then analyze the output, then add the priority prop.

The automated solution skips all of that.

Now there might be a case to document it for production apps since the warning only prints during next dev and LCP could change for a production deployment.

Maybe we should direct users to use dev tools instead which will show LCP?

Does that solve the problem?

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara left review comments

@ijjk ijjk ijjk approved these changes

@huozhi huozhi Awaiting requested review from huozhi huozhi is a code owner

@leerob leerob Awaiting requested review from leerob

@lfades lfades Awaiting requested review from lfades

@padmaia padmaia Awaiting requested review from padmaia

@shuding shuding Awaiting requested review from shuding shuding is a code owner

@timneutkens timneutkens Awaiting requested review from timneutkens timneutkens is a code owner

Assignees
No one assigned
Labels
created-by: Next.js team PRs by the Next.js team type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[11.1.3-canary.104] ReferenceError: PerformanceObserver is not defined
3 participants
@styfle @ijjk @kara