fix: add missing "use client" directive to client components and styles#35719
fix: add missing "use client" directive to client components and styles#35719dmytrokirpa wants to merge 8 commits intomicrosoft:masterfrom
Conversation
- Added "use client" directive to global-context-selector.ts, useDisableBodyScroll.styles.ts, and multiple Attachment component styles. - Updated various components in react-migration-v0-v9 and react-migration-v8-v9 libraries to include "use client" for compatibility with client-side rendering. - Included "use client" in utility hooks and context files to ensure proper execution in client environments. - Adjusted imports and ensured consistent application of the directive across all affected files.
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
.claude/settings.local.json
Outdated
| @@ -0,0 +1,25 @@ | |||
| { | |||
There was a problem hiding this comment.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 614 | Changed |
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 620 | Changed |
vr-tests-react-components/TagPicker 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/TagPicker.disabled - Dark Mode.disabled input hover.chromium.png | 658 | Changed |
vr-tests-web-components/TextInput 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-web-components/TextInput. - Dark Mode.normal.chromium_1.png | 288 | Changed |
vr-tests/Callout 10 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests/Callout.Bottom left edge - RTL.default.chromium.png | 2199 | Changed |
| vr-tests/Callout.Beak 25.default.chromium.png | 2198 | Changed |
| vr-tests/Callout.Bottom right edge - RTL.default.chromium.png | 1124 | Changed |
| vr-tests/Callout.Gap space 25.default.chromium.png | 2195 | Changed |
| vr-tests/Callout.No callout width specified.default.chromium.png | 2143 | Changed |
| vr-tests/Callout.Right bottom edge.default.chromium.png | 3095 | Changed |
| vr-tests/Callout.Right center.default.chromium.png | 2117 | Changed |
| vr-tests/Callout.Rendering callout attached to a rectangle.default.chromium.png | 1835 | Changed |
| vr-tests/Callout.Root.default.chromium.png | 2195 | Changed |
| vr-tests/Callout.Top auto edge.default.chromium.png | 2212 | Changed |
vr-tests/Keytip 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests/Keytip.Offset.default.chromium.png | 86 | Changed |
| vr-tests/Keytip.Root.default.chromium.png | 55 | Changed |
vr-tests/react-charting-AreaChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests/react-charting-AreaChart.Custom Accessibility.default.chromium.png | 11 | Changed |
vr-tests/react-charting-LineChart 4 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests/react-charting-LineChart.Multiple - RTL.default.chromium.png | 200 | Changed |
| vr-tests/react-charting-LineChart.Multiple - Dark Mode.default.chromium.png | 181 | Changed |
| vr-tests/react-charting-LineChart.Events.default.chromium.png | 2 | Changed |
| vr-tests/react-charting-LineChart.Multiple.default.chromium.png | 192 | Changed |
vr-tests/react-charting-VerticalBarChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests/react-charting-VerticalBarChart.Basic - Secondary Y Axis.default.chromium.png | 3 | Changed |
There were 2 duplicate changes discarded. Check the build logs for more information.
Hotell
left a comment
There was a problem hiding this comment.
left some comments where some need action, overall LGTM, ty
| code: `"use client";\n${testCases.customHooks.importedHookReference}`, | ||
| }, | ||
| { | ||
| name: 'Imported RSC-unsface function reference with "use client" directive', |
There was a problem hiding this comment.
| name: 'Imported RSC-unsface function reference with "use client" directive', | |
| name: 'Imported RSC-unsafe function reference with "use client" directive', |
| /** | ||
| * Track imported custom hooks and RSC-unsface functions | ||
| */ | ||
| ImportDeclaration(node: TSESTree.ImportDeclaration) { |
There was a problem hiding this comment.
can we merge this with Identifier visitor logic ? as we are already checking if its coming from import
| return; | ||
| } | ||
|
|
||
| // Skip if this identifier is in a type-only position |
There was a problem hiding this comment.
can we improve the reasoning here ? isn't this merely defensive check to satisfy TS error ?
| }; | ||
| `, | ||
| importedHookReference: `import * as React from 'react'; | ||
| import { createPreset } from '../createPreset'; |
There was a problem hiding this comment.
we should update this test case to reflect what it is actually testing.
the createPreset which internally calls various apis (including forwardRef) which should enforce use client is churn which not important ( not being caught by the rule ).
| displayName: 'Body1', | ||
| }); | ||
| `, | ||
| importedSSRUnsafeReference: `import { createPreset } from '../createPreset'; |
| * These functions may have typeof checks internally, but when called at module scope they still | ||
| * require client-side execution. | ||
| */ | ||
| const RSC_UNSAFE_FUNCTIONS = new Set([ |
There was a problem hiding this comment.
we can keep it here for now but it breaks the generic approach as it makes it tightly coupled to our apis.
having this as configurable option when specifing the rule would make it better
There was a problem hiding this comment.
for future in order to make this rule generic and catch all corners we will need to switch to dependency tree traversal ( similarily to https://github.com/microsoft/rnx-kit/blob/main/packages/eslint-plugin/src/rules/no-export-all.js ) or to use type program and determine from TS Tree if the whole program contains api that need use client.
| @@ -1,9 +1,8 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
is this getting marked because of the hardcoded canUseDOM ? if yes we should extend the rule to also check for window.* and document.* browser only usages and flag accordingly
Previous Behavior
Multiple Fluent UI v9 packages were missing the
'use client'directive in their compiled JavaScript output, causing React Server Components (RSC) compatibility issues. Components that use client-side APIs (hooks, DOM manipulation, browser APIs) were being treated as server components by default, leading to runtime errors when developers tried to use them in RSC environments.Affected packages included:
@fluentui/react-text- Text preset components and style hooks@fluentui/react-utilities- Client-side hooks likeuseIsomorphicLayoutEffect@fluentui/react-shared-contexts- Context files requiring client-side rendering@fluentui/react-message-bar@fluentui/react-motion-components-previewgetWindow()and other browser APIsNew Behavior
All client-side components, hooks, and utilities now include the
'use client'directive in their source files. This ensures that:Files updated include:
getWindowthat require client-side executionRelated Issue(s)