[ssr] Don't assign DOM shim window.global to window #2120

aomarks · 2021-09-01T16:16:53Z

This means that globalThis.global will retain its Node built-ins, and globalThis.global === globalThis will hold, whereas before it would lose anything we didn't explicitly set on window, so something like global.Promise would be undefined.

This window.global assignment seems to have been done for compatibility with node-fetch, but I tried a fetch from the demo/global/module.js module, and the fetch seemed to work fine. Not sure if there is another invocation path I missed, but it seems ok?

Fixes #2118

cc @fetherston

…window This means that globalThis.global will retain its Node built-ins, whereas before it would lose anything we didn't explicitly set on window. Fixes #2118

changeset-bot · 2021-09-01T16:16:55Z

🦋 Changeset detected

Latest commit: 32a0307

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

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

github-actions · 2021-09-01T16:17:15Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -2% - +5% (-0.56ms - +1.56ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -3% - +3% (-3.11ms - +3.08ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -2% - +3% (-0.78ms - +1.14ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +5% (-0.57ms - +0.65ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +3% (-1.62ms - +1.75ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +1% (-1.68ms - +0.59ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -3% - +2% (-20.83ms - +18.37ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -2% - +4% (-2.32ms - +3.44ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +1% (-16.75ms - +6.14ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +4% (-4.52ms - +4.75ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +2% (-22.82ms - +13.92ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -3% - +2% (-26.95ms - +17.73ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +2% (-21.00ms - +21.90ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1844
Commit: e0801f0

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	96.87ms - 101.77ms	-	unsure 🔍 -3% - +3% -3.11ms - +3.08ms	faster ✔ 17% - 24% 20.57ms - 30.34ms
tip-of-tree tip-of-tree	97.44ms - 101.23ms	unsure 🔍 -3% - +3% -3.08ms - +3.11ms	-	faster ✔ 17% - 23% 20.81ms - 30.07ms
previous-release previous-release	120.55ms - 129.00ms	slower ❌ 20% - 31% 20.57ms - 30.34ms	slower ❌ 21% - 30% 20.81ms - 30.07ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	764.76ms - 793.02ms	-	unsure 🔍 -3% - +2% -20.83ms - +18.37ms	faster ✔ 7% - 13% 62.07ms - 109.59ms
tip-of-tree tip-of-tree	766.54ms - 793.70ms	unsure 🔍 -2% - +3% -18.37ms - +20.83ms	-	faster ✔ 7% - 12% 61.17ms - 108.03ms
previous-release previous-release	845.62ms - 883.81ms	slower ❌ 8% - 14% 62.07ms - 109.59ms	slower ❌ 8% - 14% 61.17ms - 108.03ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	811.58ms - 844.83ms	-	unsure 🔍 -3% - +2% -26.95ms - +17.73ms	faster ✔ 1% - 7% 8.99ms - 57.35ms
tip-of-tree tip-of-tree	817.90ms - 847.74ms	unsure 🔍 -2% - +3% -17.73ms - +26.95ms	-	faster ✔ 1% - 6% 5.52ms - 51.60ms
previous-release previous-release	843.82ms - 878.94ms	slower ❌ 1% - 7% 8.99ms - 57.35ms	slower ❌ 1% - 6% 5.52ms - 51.60ms	-

lit-html-kitchen-sink

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1844
Commit: e0801f0

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	36.51ms - 37.92ms	-	unsure 🔍 -2% - +3% -0.78ms - +1.14ms	faster ✔ 18% - 22% 8.19ms - 10.54ms
tip-of-tree tip-of-tree	36.38ms - 37.69ms	unsure 🔍 -3% - +2% -1.14ms - +0.78ms	-	faster ✔ 18% - 23% 8.40ms - 10.70ms
previous-release previous-release	45.64ms - 47.53ms	slower ❌ 22% - 29% 8.19ms - 10.54ms	slower ❌ 22% - 29% 8.40ms - 10.70ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	91.77ms - 95.93ms	-	unsure 🔍 -2% - +4% -2.32ms - +3.44ms	faster ✔ 5% - 10% 5.24ms - 10.78ms
tip-of-tree tip-of-tree	91.29ms - 95.29ms	unsure 🔍 -4% - +2% -3.44ms - +2.32ms	-	faster ✔ 6% - 11% 5.86ms - 11.28ms
previous-release previous-release	100.03ms - 103.69ms	slower ❌ 5% - 12% 5.24ms - 10.78ms	slower ❌ 6% - 12% 5.86ms - 11.28ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.93ms - 33.47ms	-	unsure 🔍 -2% - +5% -0.56ms - +1.56ms	unsure 🔍 -3% - +4% -1.07ms - +1.15ms
tip-of-tree tip-of-tree	31.46ms - 32.92ms	unsure 🔍 -5% - +2% -1.56ms - +0.56ms	-	unsure 🔍 -5% - +2% -1.55ms - +0.62ms
previous-release previous-release	31.86ms - 33.46ms	unsure 🔍 -4% - +3% -1.15ms - +1.07ms	unsure 🔍 -2% - +5% -0.62ms - +1.55ms	-

lit-html-repeat

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1844
Commit: e0801f0

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	13.38ms - 14.30ms	-	unsure 🔍 -4% - +5% -0.57ms - +0.65ms	faster ✔ 6% - 12% 0.87ms - 1.89ms
tip-of-tree tip-of-tree	13.39ms - 14.21ms	unsure 🔍 -5% - +4% -0.65ms - +0.57ms	-	faster ✔ 6% - 12% 0.95ms - 1.89ms
previous-release previous-release	14.99ms - 15.45ms	slower ❌ 6% - 14% 0.87ms - 1.89ms	slower ❌ 7% - 14% 0.95ms - 1.89ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	400.63ms - 418.50ms	-	unsure 🔍 -4% - +1% -16.75ms - +6.14ms	faster ✔ 27% - 31% 157.11ms - 184.05ms
tip-of-tree tip-of-tree	407.71ms - 422.03ms	unsure 🔍 -2% - +4% -6.14ms - +16.75ms	-	faster ✔ 27% - 30% 152.91ms - 177.64ms
previous-release previous-release	570.06ms - 590.23ms	slower ❌ 38% - 46% 157.11ms - 184.05ms	slower ❌ 36% - 43% 152.91ms - 177.64ms	-

lit-html-template-heavy

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1844
Commit: e0801f0

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	64.36ms - 66.63ms	-	unsure 🔍 -2% - +3% -1.62ms - +1.75ms	faster ✔ 16% - 20% 12.39ms - 15.85ms
tip-of-tree tip-of-tree	64.19ms - 66.67ms	unsure 🔍 -3% - +2% -1.75ms - +1.62ms	-	faster ✔ 16% - 20% 12.38ms - 15.99ms
previous-release previous-release	78.31ms - 80.92ms	slower ❌ 19% - 24% 12.39ms - 15.85ms	slower ❌ 19% - 25% 12.38ms - 15.99ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	128.48ms - 135.28ms	-	unsure 🔍 -3% - +4% -4.52ms - +4.75ms	faster ✔ 9% - 15% 13.69ms - 22.80ms
tip-of-tree tip-of-tree	128.62ms - 134.91ms	unsure 🔍 -4% - +3% -4.75ms - +4.52ms	-	faster ✔ 9% - 15% 13.99ms - 22.73ms
previous-release previous-release	147.10ms - 153.16ms	slower ❌ 10% - 18% 13.69ms - 22.80ms	slower ❌ 10% - 17% 13.99ms - 22.73ms	-

reactive-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1844
Commit: e0801f0

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	66.94ms - 68.61ms	-	unsure 🔍 -2% - +1% -1.68ms - +0.59ms	unsure 🔍 -3% - +1% -2.12ms - +0.54ms
tip-of-tree tip-of-tree	67.54ms - 69.09ms	unsure 🔍 -1% - +2% -0.59ms - +1.68ms	-	unsure 🔍 -2% - +2% -1.53ms - +1.05ms
previous-release previous-release	67.53ms - 69.60ms	unsure 🔍 -1% - +3% -0.54ms - +2.12ms	unsure 🔍 -2% - +2% -1.05ms - +1.53ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	812.33ms - 838.83ms	-	unsure 🔍 -3% - +2% -22.82ms - +13.92ms	unsure 🔍 -3% - +2% -21.90ms - +14.31ms
tip-of-tree tip-of-tree	817.31ms - 842.76ms	unsure 🔍 -2% - +3% -13.92ms - +22.82ms	-	unsure 🔍 -2% - +2% -17.07ms - +18.39ms
previous-release previous-release	817.03ms - 841.72ms	unsure 🔍 -2% - +3% -14.31ms - +21.90ms	unsure 🔍 -2% - +2% -18.39ms - +17.07ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	908.91ms - 936.75ms	-	unsure 🔍 -2% - +2% -21.00ms - +21.90ms	unsure 🔍 -2% - +2% -21.73ms - +18.15ms
tip-of-tree tip-of-tree	906.06ms - 938.70ms	unsure 🔍 -2% - +2% -21.90ms - +21.00ms	-	unsure 🔍 -3% - +2% -23.93ms - +19.45ms
previous-release previous-release	910.34ms - 938.90ms	unsure 🔍 -2% - +2% -18.15ms - +21.73ms	unsure 🔍 -2% - +3% -19.45ms - +23.93ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2021-09-01T17:25:15Z

packages/labs/ssr/src/lib/dom-shim.ts


    // User-provided globals, like `require`
    ...props,
  };

  window.window = window;
-  window.global = window; // Required for node-fetch


What about node-fetch here? Should we do something like this?

Object.assign(window.global, window):

See PR description -- unless you know a reason it actually fails?

If we do need it, I think we can do window.global = globalThis

aomarks · 2021-09-01T22:36:00Z

This will go out with the next batch of Lit RC releases, probably a little later this week.

This means that globalThis.global will retain its Node built-ins, and globalThis.global === globalThis will hold, whereas before it would lose anything we didn't explicitly set on window, so something like global.Promise would be undefined. This window.global assignment seems to have been done for compatibility with node-fetch, but I tried a fetch from the demo/global/module.js module, and the fetch seemed to work fine. Not sure if there is another invocation path I missed, but it seems ok? Fixes lit#2118

Don't assign DOM shim window.global (and hence globalThis.global) to …

235341f

…window This means that globalThis.global will retain its Node built-ins, whereas before it would lose anything we didn't explicitly set on window. Fixes #2118

aomarks requested a review from kevinpschaaf September 1, 2021 16:16

aomarks requested a review from justinfagnani as a code owner September 1, 2021 16:16

google-cla bot added the cla: yes label Sep 1, 2021

Add changeset

32a0307

justinfagnani reviewed Sep 1, 2021

View reviewed changes

justinfagnani approved these changes Sep 1, 2021

View reviewed changes

aomarks merged commit 2043eb0 into main Sep 1, 2021

aomarks deleted the ssr-global branch September 1, 2021 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ssr] Don't assign DOM shim window.global to window #2120

[ssr] Don't assign DOM shim window.global to window #2120

aomarks commented Sep 1, 2021 •

edited

changeset-bot bot commented Sep 1, 2021 •

edited

github-actions bot commented Sep 1, 2021 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani Sep 1, 2021

aomarks Sep 1, 2021

aomarks commented Sep 1, 2021

[ssr] Don't assign DOM shim window.global to window #2120

[ssr] Don't assign DOM shim window.global to window #2120

Conversation

aomarks commented Sep 1, 2021 • edited

changeset-bot bot commented Sep 1, 2021 • edited

🦋 Changeset detected

github-actions bot commented Sep 1, 2021 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani Sep 1, 2021

Choose a reason for hiding this comment

aomarks Sep 1, 2021

Choose a reason for hiding this comment

aomarks commented Sep 1, 2021

aomarks commented Sep 1, 2021 •

edited

changeset-bot bot commented Sep 1, 2021 •

edited

github-actions bot commented Sep 1, 2021 •

edited