-
Notifications
You must be signed in to change notification settings - Fork 2.9k
chore: mark react-shared-context exports as unstable #23325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: mark react-shared-context exports as unstable #23325
Conversation
1. updates dependencies 2. removes re-export from react-provider 3. adds re-export from react-components 4. marks Provider, Type and hooks as unstable
|
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 cd8e73e:
|
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1282 | 1240 | 5000 | |
| Button | mount | 788 | 801 | 5000 | |
| FluentProvider | mount | 2289 | 2374 | 5000 | |
| FluentProviderWithTheme | mount | 420 | 442 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 370 | 361 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 450 | 444 | 10 | |
| MakeStyles | mount | 2037 | 2041 | 50000 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 4ed24c29ddf8de037e4402d67c820b7398eaa265 (build) |
|
@bsunderhus let's hold off on publishing this until we get to an agreement of our public API |
3402b54 to
5a3a8f3
Compare
5a3a8f3 to
86972a8
Compare
packages/react-components/react-components/etc/react-components.api.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-components/etc/react-components.api.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Oleksandr Fediashov <[email protected]>
| } from '@fluentui/react-theme'; | ||
| export { useThemeClassName } from '@fluentui/react-shared-contexts'; | ||
| export { | ||
| useFluent_unstable as useFluent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we exporting those unstable apis from "stable" package ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are part of react-components public API but we plan to refactor/remove react-shared-contexts - therefore unstable in react-shared-contexts but stable in react-components.
This PR has been created before we discussed @internal exports so we used the unstable pattern.
| import type { ProviderContextValue } from './ProviderContext.types'; | ||
|
|
||
| export const ProviderContext = React.createContext<ProviderContextValue>({ | ||
| export type ProviderContextValue = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Did we need to get rid of the types.ts file? We usually separate types into their own files.
| @@ -1 +0,0 @@ | |||
| export * from './TooltipContext/index'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR shows the only line of this file was deleted, but the file was not deleted.
| export type { TooltipContextType } from './TooltipContext'; | ||
| export { ProviderContext, useFluent } from './ProviderContext'; | ||
| export type { ProviderContextValue } from './ProviderContext'; | ||
| export { ThemeContext as ThemeContext_unstable, ThemeProvider as ThemeProvider_unstable } from './ThemeContext'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future: We should follow up with a discussion of the best way to export unstable. Do we rename in the index to add the unstable suffix, or add to the item itself. What helps devs reading the code understand it best?
| import * as React from 'react'; | ||
| import { clamp, useControllableState, useEventCallback } from '@fluentui/react-utilities'; | ||
| import { useFluent } from '@fluentui/react-shared-contexts'; | ||
| import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as slider uses useFluent to get the theme dir, what is the plan for getting this in the future?
micahgodbolt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than being curious how we should get dir in the future, this is fine with me
* chore: mark react-shared-context exports as unstable 1. updates dependencies 2. removes re-export from react-provider 3. adds re-export from react-components 4. marks Provider, Type and hooks as unstable * Change * Updates vr-tests stories * re-exports as stable * Update packages/react-components/react-components/etc/react-components.api.md * Update packages/react-components/react-components/src/index.ts Co-authored-by: Oleksandr Fediashov <[email protected]> * Update packages/react-components/react-components/etc/react-components.api.md * update react-provider api snapshot Co-authored-by: ling1726 <[email protected]> Co-authored-by: Oleksandr Fediashov <[email protected]> Co-authored-by: Sean Monahan <[email protected]>

Current Behavior
New Behavior
Related Issue(s)
Fixes #23321