chore: disallow document and window via eslint for React
#28887
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spmonahan
commented
Aug 16, 2023
Contributor
Author
|
What I've seen from running this locally: v8Enabling this reveals 84 issues. NOTE: most issues only have a single link provided despite having multiple issues. Run AutoFill.tsxChoiceGroup.CoachmarkUsed to register for events and read measurements. Also used to test DOM relationships. ColorRectangleComboBoxDetailsListDocumentCardDropdownUsed for setTimeout and focus management FocusTrapZoneKeytipLayerListMarqueeSelectionNavOverflowSetPanelScrollablePaneSelectedItemsListSliderStickySwatchColorPickerTooltipThis is interesting because BasePickerDraggableZonecssColorpositioningSelectionZonev9Most issues are in Storybook stories. We could exclude stories from this lint rule but it seems to me stories are a good place to use this feature since they are documentation. react-shared-contextsProviderContext references react-treecreateHTMLElementWalker should probably be updated to get react-utilitiescanUseDOM references
|
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 627 | 656 | 5000 | |
| Button | mount | 332 | 327 | 5000 | |
| Field | mount | 1113 | 1123 | 5000 | |
| FluentProvider | mount | 711 | 714 | 5000 | |
| FluentProviderWithTheme | mount | 83 | 86 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 64 | 63 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 75 | 72 | 10 | |
| MakeStyles | mount | 842 | 868 | 50000 | |
| Persona | mount | 1771 | 1722 | 5000 | |
| SpinButton | mount | 1376 | 1381 | 5000 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit fe4085f:
|
Collaborator
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ListNestedPerf.default | 336 | 296 | 1.14:1 |
| PortalMinimalPerf.default | 90 | 81 | 1.11:1 |
| AccordionMinimalPerf.default | 83 | 77 | 1.08:1 |
| ButtonSlotsPerf.default | 313 | 292 | 1.07:1 |
| AvatarMinimalPerf.default | 114 | 108 | 1.06:1 |
| ListWith60ListItems.default | 382 | 359 | 1.06:1 |
| TextAreaMinimalPerf.default | 299 | 281 | 1.06:1 |
| AlertMinimalPerf.default | 160 | 153 | 1.05:1 |
| BoxMinimalPerf.default | 195 | 186 | 1.05:1 |
| FormMinimalPerf.default | 226 | 215 | 1.05:1 |
| LabelMinimalPerf.default | 218 | 207 | 1.05:1 |
| RadioGroupMinimalPerf.default | 263 | 251 | 1.05:1 |
| TextMinimalPerf.default | 204 | 195 | 1.05:1 |
| MenuMinimalPerf.default | 499 | 479 | 1.04:1 |
| CarouselMinimalPerf.default | 265 | 258 | 1.03:1 |
| DialogMinimalPerf.default | 445 | 431 | 1.03:1 |
| DropdownManyItemsPerf.default | 393 | 382 | 1.03:1 |
| HeaderMinimalPerf.default | 207 | 200 | 1.03:1 |
| ListMinimalPerf.default | 306 | 298 | 1.03:1 |
| ReactionMinimalPerf.default | 206 | 200 | 1.03:1 |
| AnimationMinimalPerf.default | 300 | 295 | 1.02:1 |
| ButtonMinimalPerf.default | 88 | 86 | 1.02:1 |
| FlexMinimalPerf.default | 155 | 152 | 1.02:1 |
| ItemLayoutMinimalPerf.default | 717 | 700 | 1.02:1 |
| MenuButtonMinimalPerf.default | 958 | 937 | 1.02:1 |
| RosterPerf.default | 1569 | 1543 | 1.02:1 |
| SegmentMinimalPerf.default | 195 | 191 | 1.02:1 |
| SkeletonMinimalPerf.default | 201 | 197 | 1.02:1 |
| IconMinimalPerf.default | 383 | 374 | 1.02:1 |
| TableManyItemsPerf.default | 1104 | 1083 | 1.02:1 |
| TreeWith60ListItems.default | 87 | 85 | 1.02:1 |
| ButtonOverridesMissPerf.default | 631 | 627 | 1.01:1 |
| ChatMinimalPerf.default | 429 | 423 | 1.01:1 |
| DatepickerMinimalPerf.default | 3515 | 3473 | 1.01:1 |
| DropdownMinimalPerf.default | 1428 | 1417 | 1.01:1 |
| LayoutMinimalPerf.default | 201 | 199 | 1.01:1 |
| SplitButtonMinimalPerf.default | 2252 | 2221 | 1.01:1 |
| TooltipMinimalPerf.default | 1266 | 1248 | 1.01:1 |
| TreeMinimalPerf.default | 473 | 469 | 1.01:1 |
| ChatDuplicateMessagesPerf.default | 153 | 153 | 1:1 |
| ProviderMergeThemesPerf.default | 632 | 630 | 1:1 |
| SliderMinimalPerf.default | 715 | 717 | 1:1 |
| StatusMinimalPerf.default | 397 | 396 | 1:1 |
| CardMinimalPerf.default | 319 | 323 | 0.99:1 |
| CheckboxMinimalPerf.default | 1122 | 1137 | 0.99:1 |
| GridMinimalPerf.default | 186 | 188 | 0.99:1 |
| HeaderSlotsPerf.default | 451 | 454 | 0.99:1 |
| InputMinimalPerf.default | 519 | 524 | 0.99:1 |
| PopupMinimalPerf.default | 339 | 343 | 0.99:1 |
| TableMinimalPerf.default | 235 | 237 | 0.99:1 |
| CustomToolbarPrototype.default | 1443 | 1452 | 0.99:1 |
| ToolbarMinimalPerf.default | 520 | 526 | 0.99:1 |
| DividerMinimalPerf.default | 200 | 204 | 0.98:1 |
| EmbedMinimalPerf.default | 1806 | 1852 | 0.98:1 |
| LoaderMinimalPerf.default | 183 | 187 | 0.98:1 |
| ProviderMinimalPerf.default | 191 | 194 | 0.98:1 |
| RefMinimalPerf.default | 111 | 115 | 0.97:1 |
| AttachmentMinimalPerf.default | 81 | 84 | 0.96:1 |
| AttachmentSlotsPerf.default | 609 | 635 | 0.96:1 |
| ChatWithPopoverPerf.default | 181 | 192 | 0.94:1 |
| ImageMinimalPerf.default | 209 | 222 | 0.94:1 |
| ListCommonPerf.default | 366 | 390 | 0.94:1 |
| VideoMinimalPerf.default | 421 | 465 | 0.91:1 |
Collaborator
📊 Bundle size report
Unchanged fixtures
|
Asset size changes
Baseline commit: 4dc9e5be60b6ce6f3f6c72c6da39bf19a504f524 (build) |
Collaborator
🕵 fluentuiv9 No visual regressions between this PR and main |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 630 | 639 | 5000 | |
| Breadcrumb | mount | 1658 | 1688 | 1000 | |
| Checkbox | mount | 1675 | 1723 | 5000 | |
| CheckboxBase | mount | 1450 | 1475 | 5000 | |
| ChoiceGroup | mount | 2854 | 2885 | 5000 | |
| ComboBox | mount | 642 | 660 | 1000 | |
| CommandBar | mount | 6223 | 6110 | 1000 | |
| ContextualMenu | mount | 12035 | 12106 | 1000 | |
| DefaultButton | mount | 729 | 759 | 5000 | |
| DetailsRow | mount | 2211 | 2125 | 5000 | |
| DetailsRowFast | mount | 2191 | 2164 | 5000 | |
| DetailsRowNoStyles | mount | 1985 | 2007 | 5000 | |
| Dialog | mount | 2797 | 2607 | 1000 | |
| DocumentCardTitle | mount | 230 | 223 | 1000 | |
| Dropdown | mount | 2007 | 1989 | 5000 | |
| FocusTrapZone | mount | 1105 | 1143 | 5000 | |
| FocusZone | mount | 1067 | 1064 | 5000 | |
| GroupedList | mount | 41609 | 41452 | 2 | |
| GroupedList | virtual-rerender | 17769 | 19829 | 2 | |
| GroupedList | virtual-rerender-with-unmount | 50501 | 50593 | 2 | |
| GroupedListV2 | mount | 233 | 230 | 2 | |
| GroupedListV2 | virtual-rerender | 214 | 215 | 2 | |
| GroupedListV2 | virtual-rerender-with-unmount | 225 | 225 | 2 | |
| IconButton | mount | 1070 | 1060 | 5000 | |
| Label | mount | 329 | 337 | 5000 | |
| Layer | mount | 2711 | 2665 | 5000 | |
| Link | mount | 401 | 402 | 5000 | |
| MenuButton | mount | 934 | 932 | 5000 | |
| MessageBar | mount | 21292 | 21253 | 5000 | |
| Nav | mount | 1906 | 1940 | 1000 | |
| OverflowSet | mount | 775 | 787 | 5000 | |
| Panel | mount | 2036 | 1787 | 1000 | |
| Persona | mount | 742 | 730 | 1000 | |
| Pivot | mount | 860 | 882 | 1000 | |
| PrimaryButton | mount | 818 | 852 | 5000 | |
| Rating | mount | 4580 | 4506 | 5000 | |
| SearchBox | mount | 896 | 917 | 5000 | |
| Shimmer | mount | 1855 | 1858 | 5000 | |
| Slider | mount | 1305 | 1322 | 5000 | |
| SpinButton | mount | 2840 | 2803 | 5000 | |
| Spinner | mount | 384 | 401 | 5000 | |
| SplitButton | mount | 1797 | 1828 | 5000 | |
| Stack | mount | 400 | 410 | 5000 | |
| StackWithIntrinsicChildren | mount | 884 | 867 | 5000 | |
| StackWithTextChildren | mount | 2630 | 2604 | 5000 | |
| SwatchColorPicker | mount | 6184 | 6118 | 5000 | |
| TagPicker | mount | 1464 | 1459 | 5000 | |
| Text | mount | 364 | 365 | 5000 | |
| TextField | mount | 938 | 911 | 5000 | |
| ThemeProvider | mount | 826 | 812 | 5000 | |
| ThemeProvider | virtual-rerender | 589 | 575 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1282 | 1260 | 5000 | |
| Toggle | mount | 626 | 602 | 5000 | |
| buttonNative | mount | 194 | 191 | 5000 |
khmakoto
approved these changes
Aug 16, 2023
spmonahan
commented
Aug 16, 2023
| '@griffel/hook-naming': 'error', | ||
| '@griffel/no-shorthands': 'error', | ||
| '@griffel/styles-file': 'error', | ||
| 'no-restricted-globals': [ |
Contributor
Author
There was a problem hiding this comment.
Probably worth considering being more restrictive. I don't believe these rules prohibit using requestAnimationFrame for example.
Contributor
Author
There was a problem hiding this comment.
Follow up question: how should we get a reference to window and document?
Fluent ships a WindowProvider package that is a React context with two hooks: useWindow() and useDocument(). This package can be used with either Fluent v8 or v9 (or anything else for that matter).
Fluent v9 has a useFluent() hook that provides access to "targetDocument". This method is specific to Fluent v9.
While useFluent() does not provide a way to get a "targetWindow", document.defaultView provides a way to access the window object for a document so perhaps we already have everything we need. This needs further investigation.
Member
v9
Yes, this should be an exception.
Good catch, indeed. FYI @bsunderhus
Yes, this should be an exception.
FYI @ling1726 |
Contributor
Author
|
Based on
are you of the opinion that we should disallow direct access to any |
Member
I don't think that we have this issue anymore, but sometime ago we had an issue when "root" document was not actually displayed and some timers ( For now, we probably can keep it as it i.e do nothing. Does it make sense? |
spmonahan
force-pushed
the
lint/disallow-window-and-document
branch
from
4aecd42 to
8503dda
Compare
github-actions
bot
modified the milestones:
August Project Cycle Q4 2023,
September Project Cycle Q4 2023
spmonahan
force-pushed
the
lint/disallow-window-and-document
branch
from
8503dda to
ac6b300
Compare
spmonahan
requested review from
a team,
ThomasMichon and
behowell
as code owners
spmonahan
force-pushed
the
lint/disallow-window-and-document
branch
2 times, most recently
from
c233dc2 to
fe4085f
Compare
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous Behavior
Lint rules allow access to global
windowanddocumentobjects in the React eslint config.New Behavior
Lint rules disallow access to global
windowanddocumentobjects in the React eslint config. This PR primarily affects FUIv8 (@fluentui/react) but also has some changes for FUIv9 (@fluentui/react-components).The gist of this change is that React components should always get references to
windowanddocumentvia context rather than accessing the global directly. However, to preserve existing behavior we will still always fall back to accessing the globals if necessary.For Fluent v8 I've introduced some new helpers:
useDocumentEx/WindowEx(): This is a thin wrapper overuseDocument/Window(), that aligns the types of these hooks with accessingdocument/windowdirectly (i.e., these new wrappers won't returnedundefinedaccording to their types). This allows these hooks to be a easy drop-in replacement fordocumentorwindowdirect access.getDocumentEx/WindowEx(): Same as the hooks above but for reading from class component Context.All React components have been updated to use these helpers. Non-React utility code has been updated to take
document/windowparameters as needed and fallback to the existinggetDocument()/getWindow()utility functions if necessary. In some cases we still directly accessdocument/windowin fallback cases to avoid cyclic dependencies.Related Issue(s)
documentandwindow#28145