[labs/observers] Prevent failing in SSR #4591

a11delavar · 2024-03-22T00:59:01Z

This PR makes observer controllers not to initialize the observers when the constructor is called in SSR environment, preventing a crash in SSR environment. Fixes #4590.

The isServer check being before window.xObserver check is intentional as window won't exist in Node. Also there is no need to warn the developer in SSR as opposed to when we are in the browser but the observer is not supported.

There is clean-up potential after #3386 has been merged, namely adding an abstract createObserver() method to the base class for all common browser observers and moving the isServer check to the base class and conditionally calling it.

changeset-bot · 2024-03-22T00:59:04Z

🦋 Changeset detected

Latest commit: 13e9db7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@lit-labs/observers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

google-cla · 2024-03-22T00:59:05Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

github-actions · 2024-03-22T00:59:15Z

📊 Tachometer Benchmark Results

Summary

⏳ Benchmarks are currently running. Results below are out of date.

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +6% (-0.36ms - +0.61ms)
^{this-change vs tip-of-tree}

render

this-change: 45.11ms - 46.80ms
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +3% (-0.68ms - +0.65ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-0.48ms - +0.79ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-0.83ms - +0.27ms)
^{this-change vs tip-of-tree}

update

this-change: 473.51ms - 480.59ms
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +7% (-1.81ms - +2.89ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-1.99ms - +0.68ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-3.34ms - +7.36ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 482.76ms - 490.39ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-5.66ms - +5.09ms)
^{this-change vs tip-of-tree}

Results

⏳ Benchmarks are currently running. Results below are out of date.

this-change

Browser: chrome-headless 122.0.6261.128
Sample size: 50
Built by: Benchmarks #6159
Commit: 196a8d4

render

Version	Avg time	vs
	45.11ms - 46.80ms	-

update

Version	Avg time	vs
	473.51ms - 480.59ms	-

update-reflect

Version	Avg time	vs
	482.76ms - 490.39ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 122.0.6261.128
Sample size: 30
Built by: Benchmarks #6159
Commit: 196a8d4

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.30ms - 19.17ms	-	unsure 🔍 -4% - +3% -0.68ms - +0.65ms	unsure 🔍 -5% - +2% -0.88ms - +0.43ms
tip-of-tree tip-of-tree	18.25ms - 19.25ms	unsure 🔍 -3% - +4% -0.65ms - +0.68ms	-	unsure 🔍 -5% - +3% -0.92ms - +0.49ms
previous-release previous-release	18.47ms - 19.46ms	unsure 🔍 -2% - +5% -0.43ms - +0.88ms	unsure 🔍 -3% - +5% -0.49ms - +0.92ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	38.12ms - 41.41ms	-	unsure 🔍 -5% - +7% -1.81ms - +2.89ms	unsure 🔍 -8% - +4% -3.38ms - +1.56ms
tip-of-tree tip-of-tree	37.54ms - 40.91ms	unsure 🔍 -7% - +5% -2.89ms - +1.81ms	-	unsure 🔍 -10% - +2% -3.95ms - +1.04ms
previous-release previous-release	38.84ms - 42.52ms	unsure 🔍 -4% - +9% -1.56ms - +3.38ms	unsure 🔍 -3% - +10% -1.04ms - +3.95ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.80ms - 11.44ms	-	unsure 🔍 -3% - +6% -0.36ms - +0.61ms	unsure 🔍 -6% - +3% -0.62ms - +0.33ms
tip-of-tree tip-of-tree	10.64ms - 11.35ms	unsure 🔍 -5% - +3% -0.61ms - +0.36ms	-	unsure 🔍 -7% - +2% -0.78ms - +0.23ms
previous-release previous-release	10.92ms - 11.62ms	unsure 🔍 -3% - +6% -0.33ms - +0.62ms	unsure 🔍 -2% - +7% -0.23ms - +0.78ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 122.0.6261.128
Sample size: 50
Built by: Benchmarks #6159
Commit: 196a8d4

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.18ms - 34.09ms	-	unsure 🔍 -1% - +2% -0.48ms - +0.79ms	unsure 🔍 -3% - +2% -0.86ms - +0.73ms
tip-of-tree tip-of-tree	33.03ms - 33.92ms	unsure 🔍 -2% - +1% -0.79ms - +0.48ms	-	unsure 🔍 -3% - +2% -1.01ms - +0.57ms
previous-release previous-release	33.04ms - 34.35ms	unsure 🔍 -2% - +3% -0.73ms - +0.86ms	unsure 🔍 -2% - +3% -0.57ms - +1.01ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	68.36ms - 69.60ms	-	unsure 🔍 -3% - +1% -1.99ms - +0.68ms	unsure 🔍 -1% - +1% -0.91ms - +1.01ms
tip-of-tree tip-of-tree	68.45ms - 70.82ms	unsure 🔍 -1% - +3% -0.68ms - +1.99ms	-	unsure 🔍 -1% - +3% -0.69ms - +2.10ms
previous-release previous-release	68.20ms - 69.66ms	unsure 🔍 -1% - +1% -1.01ms - +0.91ms	unsure 🔍 -3% - +1% -2.10ms - +0.69ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 122.0.6261.128
Sample size: 50
Built by: Benchmarks #6159
Commit: 196a8d4

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	30.51ms - 31.29ms	-	unsure 🔍 -3% - +1% -0.83ms - +0.27ms	unsure 🔍 -3% - +0% -0.83ms - +0.15ms
tip-of-tree tip-of-tree	30.80ms - 31.57ms	unsure 🔍 -1% - +3% -0.27ms - +0.83ms	-	unsure 🔍 -2% - +1% -0.54ms - +0.42ms
previous-release previous-release	30.96ms - 31.53ms	unsure 🔍 -0% - +3% -0.15ms - +0.83ms	unsure 🔍 -1% - +2% -0.42ms - +0.54ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	480.91ms - 489.55ms	-	unsure 🔍 -1% - +2% -3.34ms - +7.36ms	unsure 🔍 -1% - +1% -6.80ms - +5.09ms
tip-of-tree tip-of-tree	480.07ms - 486.39ms	unsure 🔍 -2% - +1% -7.36ms - +3.34ms	-	unsure 🔍 -2% - +0% -8.03ms - +2.30ms
previous-release previous-release	482.01ms - 490.18ms	unsure 🔍 -1% - +1% -5.09ms - +6.80ms	unsure 🔍 -0% - +2% -2.30ms - +8.03ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	490.68ms - 498.77ms	-	unsure 🔍 -1% - +1% -5.66ms - +5.09ms	unsure 🔍 -1% - +1% -6.25ms - +6.04ms
tip-of-tree tip-of-tree	491.46ms - 498.56ms	unsure 🔍 -1% - +1% -5.09ms - +5.66ms	-	unsure 🔍 -1% - +1% -5.65ms - +6.01ms
previous-release previous-release	490.20ms - 499.46ms	unsure 🔍 -1% - +1% -6.04ms - +6.25ms	unsure 🔍 -1% - +1% -6.01ms - +5.65ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani

Looks good to me. @sorvell wdyt?

github-actions · 2024-03-22T19:21:53Z

The size of lit-html.js and lit-core.min.js are as expected.

packages/labs/observers/package.json

justinfagnani

There's an important unresolved comment now

augustjk

Thanks!

TKDev7 · 2024-06-26T23:50:36Z

What is left here guys?

justinfagnani

Thanks!

Add "isServer" check in observers' constructors

2ab0099

a11delavar force-pushed the fix-observers-failing-in-ssr branch from 14f8bef to 2ab0099 Compare March 22, 2024 01:08

Add changeset

196a8d4

a11delavar marked this pull request as ready for review March 22, 2024 01:22

a11delavar requested a review from kevinpschaaf as a code owner March 22, 2024 01:22

justinfagnani requested review from justinfagnani and sorvell March 22, 2024 19:19

justinfagnani approved these changes Mar 22, 2024

View reviewed changes

a11delavar and others added 2 commits March 22, 2024 22:38

Try to fix incorrect package import causing tests to fail

3c4aed7

Merge branch 'main' into fix-observers-failing-in-ssr

2ccd17b

augustjk reviewed Apr 23, 2024

View reviewed changes

packages/labs/observers/package.json Outdated Show resolved Hide resolved

sorvell approved these changes Apr 23, 2024

View reviewed changes

justinfagnani requested changes Apr 23, 2024

View reviewed changes

Allow lit-html 3.x as a dependency as well

13e9db7

a11delavar requested a review from justinfagnani May 1, 2024 17:42

augustjk approved these changes May 1, 2024

View reviewed changes

justinfagnani approved these changes Jul 3, 2024

View reviewed changes

justinfagnani merged commit 045b6f1 into lit:main Jul 3, 2024

lit-robot mentioned this pull request Jul 31, 2024

Version Packages #4718

Merged

X-oss-byte mentioned this pull request Sep 10, 2024

[Snyk] Upgrade: , , lit X-oss-byte/pigweed#84

Open

lennartbuit mentioned this pull request Oct 21, 2024

IntersectionController not initialized in node environments #4804

Closed

[labs/observers] Prevent failing in SSR #4591

[labs/observers] Prevent failing in SSR #4591

Uh oh!

Conversation

a11delavar commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

google-cla bot commented Mar 22, 2024

Uh oh!

github-actions bot commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 22, 2024

Uh oh!

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

TKDev7 commented Jun 26, 2024

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

a11delavar commented Mar 22, 2024 •

edited

Loading

changeset-bot bot commented Mar 22, 2024 •

edited

Loading

github-actions bot commented Mar 22, 2024 •

edited

Loading