refactor: remove hard dependency of DI on FASTElement #6280

EisenbergEffect · 2022-08-15T18:17:21Z

Pull Request

📖 Description

This PR removes the import of FASTElement from the dependency injection code so that users of the DI system can tree-shake out the rest of FASTElement.

🎫 Issues

Closes refactor: remove FASTElement dependency from the DI container #6279

👩‍💻 Reviewer Notes

The import of FASTElement was removed to enable non-FASTElement users to use the dependency injection system without needing to pull in all the code for FASTElement. The DI container only had one small feature that was dependent on this for a single type check. The new code simply checks for the property it requires rather than doing a instanceof. If the needed $fastController property isn't present, a detailed error is now thrown.

📑 Test Plan

This was an internal refactor. All existing tests continue to pass.

✅ Checklist

General

I have included a change request file using $ yarn change
I have added tests for my changes.
I have tested my changes.
I have updated the project documentation to reflect my changes.
I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

Not related to this feature, but also within DI, I'm researching to see whether it would be possible to add a getAsync API so that missing dependencies in a dependency resolution tree could be asynchronously provided.

github-actions · 2022-08-15T18:18:07Z

📊 Tachometer Benchmark Results

Summary

clickTrigger10x

repeat-basic-splice-loopCount=1000&itemCount=1000&deleteCount=10&addCount=10: unsure 🔍 -6% - +3% (-11.09ms - +6.30ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-push-loopCount=200&itemCount=200: unsure 🔍 -0% - +0% (-14.34ms - +17.90ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-reverse-loopCount=200&itemCount=200: unsure 🔍 -1% - +2% (-67.50ms - +108.30ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-shift-loopCount=200&itemCount=200: unsure 🔍 -1% - +2% (-36.60ms - +87.63ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-unshift-loopCount=200&itemCount=200&addCount=1: unsure 🔍 -2% - +4% (-78.82ms - +149.11ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

create10k

render-create10k: unsure 🔍 -5% - +3% (-10.39ms - +6.07ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

createDelete5x

render-createDelete5x: unsure 🔍 -1% - +1% (-3.57ms - +2.31ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

runFile1k

observable-runFile1k: unsure 🔍 -32% - +8% (-3.35ms - +0.95ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

update10th

render-update10th: unsure 🔍 -6% - +5% (-12.28ms - +9.64ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

usedJSHeapSize

observable-runFile1k: unsure 🔍 -0% - +2% (-0.23ms - +0.98ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
render-create10k: unsure 🔍 -0% - +0% (-0.06ms - +0.04ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
render-createDelete5x: unsure 🔍 +0% - +0% (+0.02ms - +0.10ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
render-update10th: unsure 🔍 -0% - +0% (-0.03ms - +0.09ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-basic-splice-loopCount=1000&itemCount=1000&deleteCount=10&addCount=10: unsure 🔍 -0% - +0% (-0.05ms - +0.05ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-push-loopCount=200&itemCount=200: unsure 🔍 -0% - +0% (-0.03ms - +0.01ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-reverse-loopCount=200&itemCount=200: unsure 🔍 -0% - +0% (-0.04ms - +0.03ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-shift-loopCount=200&itemCount=200: unsure 🔍 -0% - +0% (-0.03ms - +0.02ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}
repeat-nested-unshift-loopCount=200&itemCount=200&addCount=1: unsure 🔍 -0% - +0% (-0.02ms - +0.02ms)
^{users/eisenbergeffect/remove-de-dep-from-di vs master Customize summary}

Results

observable-runFile1k

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

runFile1k

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	8.66ms - 11.68ms	-	unsure 🔍 -12% - +39% -0.95ms - +3.35ms
users/eisenbergeffect/remove-de-dep-from-di	7.43ms - 10.50ms	unsure 🔍 -32% - +8% -3.35ms - +0.95ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	47.88ms - 48.74ms	-	unsure 🔍 -2% - +0% -0.98ms - +0.23ms
users/eisenbergeffect/remove-de-dep-from-di	48.25ms - 49.11ms	unsure 🔍 -0% - +2% -0.23ms - +0.98ms	-

render-create10k

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

create10k

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	204.36ms - 216.58ms	-	unsure 🔍 -3% - +5% -6.07ms - +10.39ms
users/eisenbergeffect/remove-de-dep-from-di	202.79ms - 213.83ms	unsure 🔍 -5% - +3% -10.39ms - +6.07ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	47.03ms - 47.11ms	-	unsure 🔍 -0% - +0% -0.04ms - +0.06ms
users/eisenbergeffect/remove-de-dep-from-di	47.03ms - 47.08ms	unsure 🔍 -0% - +0% -0.06ms - +0.04ms	-

render-createDelete5x

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

createDelete5x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	322.13ms - 326.47ms	-	unsure 🔍 -1% - +1% -2.31ms - +3.57ms
users/eisenbergeffect/remove-de-dep-from-di	321.68ms - 325.65ms	unsure 🔍 -1% - +1% -3.57ms - +2.31ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	53.60ms - 53.65ms	-	unsure 🔍 -0% - -0% -0.10ms - -0.02ms
users/eisenbergeffect/remove-de-dep-from-di	53.65ms - 53.71ms	unsure 🔍 +0% - +0% +0.02ms - +0.10ms	-

render-update10th

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

update10th

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	184.07ms - 197.87ms	-	unsure 🔍 -5% - +6% -9.64ms - +12.28ms
users/eisenbergeffect/remove-de-dep-from-di	181.14ms - 198.17ms	unsure 🔍 -6% - +5% -12.28ms - +9.64ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	46.99ms - 47.08ms	-	unsure 🔍 -0% - +0% -0.09ms - +0.03ms
users/eisenbergeffect/remove-de-dep-from-di	47.03ms - 47.11ms	unsure 🔍 -0% - +0% -0.03ms - +0.09ms	-

repeat-basic-splice-loopCount=1000&itemCount=1000&deleteCount=10&addCount=10

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

clickTrigger10x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	191.60ms - 203.08ms	-	unsure 🔍 -3% - +6% -6.30ms - +11.09ms
users/eisenbergeffect/remove-de-dep-from-di	188.42ms - 201.47ms	unsure 🔍 -6% - +3% -11.09ms - +6.30ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	50.75ms - 50.82ms	-	unsure 🔍 -0% - +0% -0.05ms - +0.05ms
users/eisenbergeffect/remove-de-dep-from-di	50.75ms - 50.82ms	unsure 🔍 -0% - +0% -0.05ms - +0.05ms	-

repeat-nested-push-loopCount=200&itemCount=200

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

clickTrigger10x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	3620.85ms - 3642.81ms	-	unsure 🔍 -0% - +0% -17.90ms - +14.34ms
users/eisenbergeffect/remove-de-dep-from-di	3621.81ms - 3645.41ms	unsure 🔍 -0% - +0% -14.34ms - +17.90ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	91.04ms - 91.07ms	-	unsure 🔍 -0% - +0% -0.01ms - +0.03ms
users/eisenbergeffect/remove-de-dep-from-di	91.03ms - 91.06ms	unsure 🔍 -0% - +0% -0.03ms - +0.01ms	-

repeat-nested-reverse-loopCount=200&itemCount=200

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

clickTrigger10x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	5565.94ms - 5664.25ms	-	unsure 🔍 -2% - +1% -108.30ms - +67.50ms
users/eisenbergeffect/remove-de-dep-from-di	5562.63ms - 5708.36ms	unsure 🔍 -1% - +2% -67.50ms - +108.30ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	130.98ms - 131.03ms	-	unsure 🔍 -0% - +0% -0.03ms - +0.04ms
users/eisenbergeffect/remove-de-dep-from-di	130.97ms - 131.02ms	unsure 🔍 -0% - +0% -0.04ms - +0.03ms	-

repeat-nested-shift-loopCount=200&itemCount=200

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

clickTrigger10x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	3575.22ms - 3637.86ms	-	unsure 🔍 -2% - +1% -87.63ms - +36.60ms
users/eisenbergeffect/remove-de-dep-from-di	3578.41ms - 3685.69ms	unsure 🔍 -1% - +2% -36.60ms - +87.63ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	91.04ms - 91.08ms	-	unsure 🔍 -0% - +0% -0.02ms - +0.03ms
users/eisenbergeffect/remove-de-dep-from-di	91.04ms - 91.07ms	unsure 🔍 -0% - +0% -0.03ms - +0.02ms	-

repeat-nested-unshift-loopCount=200&itemCount=200&addCount=1

Browser: chrome-headless 104.0.5112.79
Sample size: 30
Built by: Validate Benchmarks #169
Commit: cb4de6d

clickTrigger10x

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	3684.86ms - 3738.52ms	-	unsure 🔍 -4% - +2% -149.11ms - +78.82ms
users/eisenbergeffect/remove-de-dep-from-di	3636.07ms - 3857.60ms	unsure 🔍 -2% - +4% -78.82ms - +149.11ms	-

usedJSHeapSize

Version	Avg time	vs master	vs users/eisenbergeffect/remove-de-dep-from-di
master	90.99ms - 91.03ms	-	unsure 🔍 -0% - +0% -0.02ms - +0.02ms
users/eisenbergeffect/remove-de-dep-from-di	91.00ms - 91.01ms	unsure 🔍 -0% - +0% -0.02ms - +0.02ms	-

_{tachometer-reporter-action v2 for Validate Benchmarks}

change/@microsoft-fast-element-953ae13a-b331-48b2-b410-0881b9fcfce2.json

Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com>

…dep-from-di' into users/eisenbergeffect/remove-de-dep-from-di

chrisdholt

This is great, thanks @EisenbergEffect

* refactor: remove dependency of DI on FASTElement * feat: throw if attempting to respect DOM connect without a FASTElement * Change files * fix change file Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com> * chore: fix prettier missing comma in enum Co-authored-by: EisenbergEffect <roeisenb@microsoft.com> Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com>

EisenbergEffect and others added 3 commits August 15, 2022 11:19

refactor: remove dependency of DI on FASTElement

db7ece6

feat: throw if attempting to respect DOM connect without a FASTElement

de09280

Change files

b4098d7

EisenbergEffect requested review from chrisdholt, janechu and nicholasrice as code owners August 15, 2022 18:17

nicholasrice reviewed Aug 15, 2022

View reviewed changes

change/@microsoft-fast-element-953ae13a-b331-48b2-b410-0881b9fcfce2.json Outdated Show resolved Hide resolved

nicholasrice approved these changes Aug 15, 2022

View reviewed changes

fix change file

fd6ecf4

Co-authored-by: Nicholas Rice <3213292+nicholasrice@users.noreply.github.com>

robarbms approved these changes Aug 16, 2022

View reviewed changes

EisenbergEffect and others added 3 commits August 15, 2022 21:43

Merge branch 'master' into users/eisenbergeffect/remove-de-dep-from-di

9502449

chore: fix prettier missing comma in enum

4607cbd

Merge remote-tracking branch 'origin/users/eisenbergeffect/remove-de-…

2d6c9c0

…dep-from-di' into users/eisenbergeffect/remove-de-dep-from-di

chrisdholt approved these changes Aug 16, 2022

View reviewed changes

chrisdholt merged commit ac3954c into master Aug 16, 2022

chrisdholt deleted the users/eisenbergeffect/remove-de-dep-from-di branch August 16, 2022 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove hard dependency of DI on FASTElement #6280

refactor: remove hard dependency of DI on FASTElement #6280

EisenbergEffect commented Aug 15, 2022

github-actions bot commented Aug 15, 2022 •

edited

Loading

📊 Tachometer Benchmark Results

Summary

clickTrigger10x

create10k

createDelete5x

runFile1k

update10th

usedJSHeapSize

Results

runFile1k

usedJSHeapSize

create10k

usedJSHeapSize

createDelete5x

usedJSHeapSize

update10th

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

chrisdholt left a comment

refactor: remove hard dependency of DI on FASTElement #6280

refactor: remove hard dependency of DI on FASTElement #6280

Conversation

EisenbergEffect commented Aug 15, 2022

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

⏭ Next Steps

github-actions bot commented Aug 15, 2022 • edited Loading

📊 Tachometer Benchmark Results

Summary

clickTrigger10x

create10k

createDelete5x

runFile1k

update10th

usedJSHeapSize

Results

runFile1k

usedJSHeapSize

create10k

usedJSHeapSize

createDelete5x

usedJSHeapSize

update10th

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

clickTrigger10x

usedJSHeapSize

chrisdholt left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2022 •

edited

Loading