Skip to content
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

Adding lib-commonjs top-level entries to exports map #24792

Merged
merged 7 commits into from
Sep 15, 2022
Merged

Conversation

dzearing
Copy link
Member

@dzearing dzearing commented Sep 14, 2022

This change adds lib-commonjs entries to the @fluentui/react exports map. It also disables the now-obsolete transform which is renaming /lib/ to /lib-commonjs/ for cjs files. Requiring a /lib/ top level entry should direct to the appropriate module format, thanks to the exports map.

There are 2 cases where @fluentui/react/lib-commonjs/* imports are used:

  1. Jest tests - some repos use Fluent components in jest tests, and still use commonjs. In this case, the test itself may import a Fluent component, and needs the cjs flavor.
  2. SSR scenarios - in server rendered cases, it might be required to live in a CJS-only environment. There have been past issues of people trying to use the components in node server land, only to realize they need to bundle first. This would enable a cjs node script to import a fluent component via lib-commonjs paths.

The specific case where a partner hit this was in this jest cjs module:

const Charting = require('react-charting');

This will cause the cjs index.js entry to be resolved. This traverses to a file which imports @fluentui/lib/Utilities. Our transform would have renamed this to /lib-commonjs/Utilities in the cjs source, which isn't a valid import.

Removing the postprocess-commonjs task means that the import stays /lib/Utilities, which can be redirected to cjs or esm thanks to the exports map.

But also, we're adding /lib-commonjs/* entries to the exports map. If charting package doesn't get re-published (it won't without a change file), it will still try and import this path. Now with the updated exports map, this will direct to the right path.

@dzearing dzearing requested a review from a team as a code owner September 14, 2022 17:02
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 14, 2022

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 5456327:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 14, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: b111dddd3dab26c409857723a6e34810a60c41b2 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext
533 B
341 B
global-context
createContextSelector
554 B
348 B
priority-overflow
createOverflowManager
2.936 kB
1.212 kB
react-accordion
Accordion (including children components)
78.914 kB
24.06 kB
react-alert
Alert
83.511 kB
20.921 kB
react-avatar
Avatar
48.381 kB
13.696 kB
react-avatar
AvatarGroup
14.95 kB
5.989 kB
react-avatar
AvatarGroupItem
68.349 kB
19.039 kB
react-badge
Badge
22.6 kB
7.205 kB
react-badge
CounterBadge
23.503 kB
7.497 kB
react-badge
PresenceBadge
24.05 kB
7.067 kB
react-button
Button
36.119 kB
9.647 kB
react-button
CompoundButton
43.144 kB
10.86 kB
react-button
MenuButton
38.813 kB
10.551 kB
react-button
SplitButton
46.228 kB
11.933 kB
react-button
ToggleButton
51.888 kB
11.127 kB
react-card
Card - All
67.002 kB
19.261 kB
react-card
Card
62.684 kB
18.177 kB
react-card
CardFooter
8.561 kB
3.601 kB
react-card
CardHeader
9.604 kB
3.94 kB
react-card
CardPreview
8.662 kB
3.656 kB
react-combobox
Combobox (including child components)
74.636 kB
24.186 kB
react-combobox
Dropdown (including child components)
74.236 kB
24.086 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
188.001 kB
52.069 kB
react-components
react-components: FluentProvider & webLightTheme
33.394 kB
11.007 kB
react-dialog
Dialog (including children components)
81.048 kB
24.208 kB
react-divider
Divider
16.459 kB
5.902 kB
react-image
Image
10.78 kB
4.264 kB
react-input
Input
23.757 kB
7.704 kB
react-label
Label
9.338 kB
3.86 kB
react-link
Link
11.784 kB
4.867 kB
react-menu
Menu (including children components)
115.778 kB
35.425 kB
react-menu
Menu (including selectable components)
118.977 kB
35.92 kB
react-overflow
hooks only
10.685 kB
4.104 kB
react-popover
Popover
102.963 kB
31.553 kB
react-portal
Portal
10.576 kB
3.875 kB
react-portal-compat
PortalCompatProvider
5.851 kB
1.964 kB
react-positioning
usePositioning
19.7 kB
7.404 kB
react-provider
FluentProvider
15.755 kB
5.883 kB
react-radio
Radio
35.56 kB
11.929 kB
react-radio
RadioGroup
14.248 kB
5.7 kB
react-select
Select
20.846 kB
7.346 kB
react-slider
Slider
31.526 kB
10.046 kB
react-spinbutton
SpinButton
44.102 kB
12.425 kB
react-spinner
Spinner
19.977 kB
6.438 kB
react-switch
Switch
32.097 kB
10.27 kB
react-text
Text - Default
11.782 kB
4.605 kB
react-text
Text - Wrappers
15.092 kB
5.044 kB
react-textarea
Textarea
23.988 kB
8.011 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
29.65 kB
6.444 kB
react-theme
Teams: Light theme
17.486 kB
5.057 kB
react-tooltip
Tooltip
41.502 kB
14.623 kB
react-utilities
SSRProvider
180 B
159 B
🤖 This report was generated against b111dddd3dab26c409857723a6e34810a60c41b2

@spmonahan
Copy link
Contributor

SSR scenarios - in server rendered cases, it might be required to live in a CJS-only environment. There have been past issues of people trying to use the components in node server land, only to realize they need to bundle first. This would enable a cjs node script to import a fluent component via lib-commonjs paths.

Can you expand on why this is necessary? It seems like the existing exports map already lets Node users require() the CJS build without bundling?

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 126 117 1.08:1
BoxMinimalPerf.default 290 274 1.06:1
ButtonMinimalPerf.default 137 129 1.06:1
FlexMinimalPerf.default 244 230 1.06:1
GridMinimalPerf.default 281 266 1.06:1
TextMinimalPerf.default 291 275 1.06:1
AccordionMinimalPerf.default 120 115 1.04:1
CarouselMinimalPerf.default 383 370 1.04:1
ChatWithPopoverPerf.default 299 288 1.04:1
HeaderSlotsPerf.default 636 613 1.04:1
ListWith60ListItems.default 522 500 1.04:1
SkeletonMinimalPerf.default 287 275 1.04:1
AvatarMinimalPerf.default 154 150 1.03:1
ButtonSlotsPerf.default 455 443 1.03:1
ImageMinimalPerf.default 321 312 1.03:1
ItemLayoutMinimalPerf.default 984 957 1.03:1
RadioGroupMinimalPerf.default 369 358 1.03:1
RefMinimalPerf.default 181 176 1.03:1
SegmentMinimalPerf.default 286 279 1.03:1
AttachmentSlotsPerf.default 931 913 1.02:1
DialogMinimalPerf.default 649 634 1.02:1
DividerMinimalPerf.default 294 287 1.02:1
LayoutMinimalPerf.default 288 282 1.02:1
RosterPerf.default 1781 1747 1.02:1
PopupMinimalPerf.default 527 518 1.02:1
PortalMinimalPerf.default 142 139 1.02:1
ProviderMinimalPerf.default 335 329 1.02:1
ReactionMinimalPerf.default 312 307 1.02:1
AnimationMinimalPerf.default 439 436 1.01:1
ChatMinimalPerf.default 609 601 1.01:1
DropdownManyItemsPerf.default 551 546 1.01:1
EmbedMinimalPerf.default 3015 2990 1.01:1
InputMinimalPerf.default 948 939 1.01:1
ListCommonPerf.default 503 496 1.01:1
ListNestedPerf.default 461 457 1.01:1
LoaderMinimalPerf.default 558 550 1.01:1
IconMinimalPerf.default 555 551 1.01:1
TableMinimalPerf.default 326 323 1.01:1
ToolbarMinimalPerf.default 770 762 1.01:1
CheckboxMinimalPerf.default 1723 1720 1:1
DropdownMinimalPerf.default 2255 2245 1:1
HeaderMinimalPerf.default 291 290 1:1
MenuMinimalPerf.default 687 688 1:1
MenuButtonMinimalPerf.default 1404 1401 1:1
SliderMinimalPerf.default 1306 1302 1:1
SplitButtonMinimalPerf.default 3621 3616 1:1
TreeMinimalPerf.default 669 668 1:1
ButtonOverridesMissPerf.default 1075 1082 0.99:1
CardMinimalPerf.default 437 440 0.99:1
ListMinimalPerf.default 415 421 0.99:1
ProviderMergeThemesPerf.default 1054 1060 0.99:1
StatusMinimalPerf.default 549 552 0.99:1
TableManyItemsPerf.default 1540 1563 0.99:1
TextAreaMinimalPerf.default 376 379 0.99:1
CustomToolbarPrototype.default 2243 2258 0.99:1
TooltipMinimalPerf.default 1918 1939 0.99:1
VideoMinimalPerf.default 600 607 0.99:1
DatepickerMinimalPerf.default 4739 4822 0.98:1
FormMinimalPerf.default 302 307 0.98:1
ChatDuplicateMessagesPerf.default 231 237 0.97:1
LabelMinimalPerf.default 306 318 0.96:1
AlertMinimalPerf.default 212 228 0.93:1
TreeWith60ListItems.default 118 133 0.89:1

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 748 780 5000
Breadcrumb mount 2352 2322 1000
Checkbox mount 2251 2233 5000
CheckboxBase mount 1959 1950 5000
ChoiceGroup mount 3819 3833 5000
ComboBox mount 758 765 1000
CommandBar mount 8898 8897 1000
ContextualMenu mount 9742 9637 1000
DefaultButton mount 930 940 5000
DetailsRow mount 3027 2930 5000
DetailsRowFast mount 2964 2931 5000
DetailsRowNoStyles mount 2848 2828 5000
Dialog mount 2560 2543 1000
DocumentCardTitle mount 163 137 1000
Dropdown mount 2756 2761 5000
FocusTrapZone mount 1529 1512 5000
FocusZone mount 1510 1517 5000
IconButton mount 1372 1383 5000
Label mount 324 319 5000
Layer mount 3711 3701 5000
Link mount 440 431 5000
MenuButton mount 1180 1199 5000
MessageBar mount 1896 1897 5000
Nav mount 2648 2639 1000
OverflowSet mount 982 978 5000
Panel mount 2095 2077 1000
Persona mount 814 824 1000
Pivot mount 1110 1099 1000
PrimaryButton mount 1062 1072 5000
Rating mount 6634 6572 5000
SearchBox mount 1055 1069 5000
Shimmer mount 2495 2510 5000
Slider mount 1679 1680 5000
SpinButton mount 3923 3865 5000
Spinner mount 389 396 5000
SplitButton mount 2438 2422 5000
Stack mount 461 481 5000
StackWithIntrinsicChildren mount 1830 1827 5000
StackWithTextChildren mount 4500 4505 5000
SwatchColorPicker mount 9015 9097 5000
TagPicker mount 1894 1925 5000
TeachingBubble mount 76611 75566 5000
Text mount 394 390 5000
TextField mount 1131 1130 5000
ThemeProvider mount 1030 1024 5000
ThemeProvider virtual-rerender 723 706 5000
ThemeProvider virtual-rerender-with-unmount 1703 1586 5000
Toggle mount 719 723 5000
buttonNative mount 110 107 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 14, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1304 1318 5000
Button mount 840 959 5000
FluentProvider mount 1477 1562 5000
FluentProviderWithTheme mount 629 623 10
FluentProviderWithTheme virtual-rerender 590 513 10
FluentProviderWithTheme virtual-rerender-with-unmount 629 631 10
MakeStyles mount 1848 1913 50000
SpinButton mount 2531 2443 5000

@sysofwan
Copy link
Contributor

Thanks @dzearing! Is it possible to create a changefile for fluentui/react-charting too? We are trying to upgrade it when encountering this issue.

@dzearing
Copy link
Member Author

dzearing commented Sep 14, 2022

@spmonahan I've updated the description with a specific scenario, let me know if that clears things up.

TLDR: importing our own react-experiments and react-charting will apply the cjs transform to cjs artifacts. That is, replace lib with lib-commonjs imports. This was done before exports maps existed. Which means if you use react-charting in cjs scenario, you will need to resolve @fluentui/react/lib-commonjs/Utilities. Which doesn't work now.

Removing the transform and republishing will fix this to import from /lib/ always, which will resolve.

And for backwards compatibility it still seems right to allow for the lib-commonjs entries to resolve correctly. The case would be something like: another library outside our repo is doing something similar and importing the cjs files directly. Keep in mind that cjs can't import esm, so this is pretty likely, especially in things like Next.js apps where there's heavy usage of SSR.

@spmonahan
Copy link
Contributor

@dzearing That makes sense. Thanks for the example!

@dzearing dzearing requested a review from a team as a code owner September 14, 2022 22:57
@Hotell
Copy link
Contributor

Hotell commented Sep 15, 2022

those snapshots fixes has been merged #24808, pls update your PR. ty

scripts/just.config.ts Show resolved Hide resolved
packages/react/package.json Show resolved Hide resolved
Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unblocking - there are 2 actionable that need to be done. ty

@dzearing dzearing enabled auto-merge (squash) September 15, 2022 17:23
@dzearing dzearing merged commit 682ead9 into master Sep 15, 2022
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 19, 2022
* master: (28 commits)
  fix: use trigger prop for aria-haspopup (microsoft#24794)
  chore(react-dialog): scaffold DialogContent component (microsoft#24844)
  chore: Northstar screener should read from screenerStates.json (microsoft#24848)
  applying package updates
  (web components) Standardize focus treatment (microsoft#24771)
  Divider - allow default prop override (microsoft#24840)
  GroupedList: fix virtualization (unstable preview) (microsoft#24460)
  fix: Add explicit children prop to TeachingBubble to support React 18 (microsoft#24823)
  feat: Adds `visible` prop to `TableCellActions` (microsoft#24831)
  [Northstar][Dropdown] Fix styling mutation when merging themes (microsoft#24787)
  fix: export `tableCellActionsClassNames` from unstable (microsoft#24830)
  bugfix(react-dialog): Adds color style to DialogSurface (microsoft#24832)
  applying package updates
  Prevent group toggling from selecting the whole group (microsoft#24822)
  feat(react-textarea): Add shadow variant of filled appearance (microsoft#24512)
  applying package updates
  Adding lib-commonjs top-level entries to exports map (microsoft#24792)
  Created shim packages (microsoft#24780)
  feat(react-menu): replace keydown handlers by useARIAButtonShorthand on MenuItem (microsoft#24738)
  fix: update version mismatches triggered by v9 release (microsoft#24812)
  ...
@khmakoto khmakoto deleted the fix-cjs-entries branch April 5, 2023 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants