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

fix: strip comments for JSX pragma #29145

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Sep 14, 2023

Previous Behavior

We started to use new JSX pragma via comments, for example:

/** @jsxRuntime automatic */
/** @jsxImportSource @fluentui/react-jsx-runtime */

The problem is that these comments appeared in our artifacts:

/** @jsxRuntime automatic */
/** @jsxImportSource @fluentui/react-jsx-runtime */
import { jsx as _jsx } from "@fluentui/react-jsx-runtime/jsx-runtime";

https://unpkg.com/browse/@fluentui/react-image@9.1.31/lib/components/Image/renderImage.js

And it causes issues if Babel is used on node_modules, see #29126 (comment)

New Behavior

image

These comments are removed during build. No comments => no problems.

Note: I haven't checked if sourcemaps still work after this change.

Related Issue(s)

Fixes #29126

@layershifter layershifter marked this pull request as ready for review September 14, 2023 08:50
@layershifter layershifter requested a review from a team as a code owner September 14, 2023 08:50
@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
InfoButton mount 15 15 5000 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 605 651 5000
Button mount 306 316 5000
Field mount 1106 1082 5000
FluentProvider mount 704 696 5000
FluentProviderWithTheme mount 76 79 10
FluentProviderWithTheme virtual-rerender 75 78 10
FluentProviderWithTheme virtual-rerender-with-unmount 78 71 10
InfoButton mount 15 15 5000 Possible regression
MakeStyles mount 855 867 50000
Persona mount 1682 1706 5000
SpinButton mount 1389 1346 5000

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TreeWith60ListItems.default 89 74 1.2:1
AvatarMinimalPerf.default 115 102 1.13:1
ButtonMinimalPerf.default 94 86 1.09:1
LayoutMinimalPerf.default 207 190 1.09:1
PortalMinimalPerf.default 88 82 1.07:1
TextMinimalPerf.default 194 182 1.07:1
RadioGroupMinimalPerf.default 263 247 1.06:1
TableManyItemsPerf.default 1133 1070 1.06:1
GridMinimalPerf.default 189 180 1.05:1
AnimationMinimalPerf.default 294 283 1.04:1
CheckboxMinimalPerf.default 1149 1108 1.04:1
ListMinimalPerf.default 317 306 1.04:1
PopupMinimalPerf.default 353 338 1.04:1
RefMinimalPerf.default 111 107 1.04:1
SegmentMinimalPerf.default 196 189 1.04:1
TextAreaMinimalPerf.default 288 277 1.04:1
TreeMinimalPerf.default 492 471 1.04:1
AttachmentMinimalPerf.default 89 86 1.03:1
BoxMinimalPerf.default 195 190 1.03:1
ButtonOverridesMissPerf.default 644 628 1.03:1
ButtonSlotsPerf.default 317 308 1.03:1
ChatMinimalPerf.default 431 418 1.03:1
ChatWithPopoverPerf.default 199 193 1.03:1
HeaderSlotsPerf.default 462 449 1.03:1
InputMinimalPerf.default 547 533 1.03:1
MenuMinimalPerf.default 503 489 1.03:1
SkeletonMinimalPerf.default 204 199 1.03:1
SliderMinimalPerf.default 743 723 1.03:1
SplitButtonMinimalPerf.default 2298 2231 1.03:1
EmbedMinimalPerf.default 1880 1835 1.02:1
ListWith60ListItems.default 370 363 1.02:1
ProviderMergeThemesPerf.default 658 644 1.02:1
ProviderMinimalPerf.default 205 201 1.02:1
StatusMinimalPerf.default 392 385 1.02:1
DatepickerMinimalPerf.default 3485 3456 1.01:1
IconMinimalPerf.default 400 395 1.01:1
DropdownMinimalPerf.default 1412 1410 1:1
ItemLayoutMinimalPerf.default 696 695 1:1
LabelMinimalPerf.default 216 217 1:1
ListCommonPerf.default 386 385 1:1
MenuButtonMinimalPerf.default 954 957 1:1
TableMinimalPerf.default 231 230 1:1
AlertMinimalPerf.default 152 154 0.99:1
DialogMinimalPerf.default 434 440 0.99:1
DropdownManyItemsPerf.default 383 388 0.99:1
FlexMinimalPerf.default 150 152 0.99:1
CustomToolbarPrototype.default 1465 1481 0.99:1
ToolbarMinimalPerf.default 520 525 0.99:1
ChatDuplicateMessagesPerf.default 145 148 0.98:1
DividerMinimalPerf.default 201 206 0.98:1
ListNestedPerf.default 317 325 0.98:1
RosterPerf.default 1515 1541 0.98:1
ReactionMinimalPerf.default 205 209 0.98:1
VideoMinimalPerf.default 418 427 0.98:1
CardMinimalPerf.default 302 311 0.97:1
HeaderMinimalPerf.default 207 213 0.97:1
TooltipMinimalPerf.default 1215 1247 0.97:1
CarouselMinimalPerf.default 246 256 0.96:1
FormMinimalPerf.default 220 228 0.96:1
LoaderMinimalPerf.default 184 192 0.96:1
AttachmentSlotsPerf.default 604 638 0.95:1
AccordionMinimalPerf.default 83 88 0.94:1
ImageMinimalPerf.default 204 226 0.9:1

@codesandbox-ci
Copy link

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

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

@fabricteam
Copy link
Collaborator

🕵 fluentuiv8 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

🕵 FluentUIV0 No visual regressions between this PR and main

@fabricteam
Copy link
Collaborator

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
global-context
createContext
510 B
330 B
global-context
createContextSelector
537 B
342 B
priority-overflow
createOverflowManager
4.162 kB
1.735 kB
react-accordion
Accordion (including children components)
91.384 kB
27.926 kB
react-alert
Alert
84.36 kB
23.094 kB
react-avatar
Avatar
49.738 kB
15.631 kB
react-avatar
AvatarGroup
18.584 kB
7.448 kB
react-avatar
AvatarGroupItem
64.39 kB
19.977 kB
react-badge
Badge
25.793 kB
8.348 kB
react-badge
CounterBadge
26.694 kB
8.658 kB
react-badge
PresenceBadge
24.75 kB
8.96 kB
react-button
Button
39.469 kB
10.758 kB
react-button
CompoundButton
46.824 kB
12.259 kB
react-button
MenuButton
43.896 kB
12.025 kB
react-button
SplitButton
51.922 kB
13.596 kB
react-button
ToggleButton
56.557 kB
12.667 kB
react-card
Card - All
91.249 kB
26.409 kB
react-card
Card
86.038 kB
24.868 kB
react-card
CardFooter
11.951 kB
5.031 kB
react-card
CardHeader
14.237 kB
5.798 kB
react-card
CardPreview
12.903 kB
5.408 kB
react-checkbox
Checkbox
35.614 kB
11.771 kB
react-combobox
Combobox (including child components)
90.383 kB
29.52 kB
react-combobox
Dropdown (including child components)
88.738 kB
29.167 kB
react-components
react-components: Button, FluentProvider & webLightTheme
69.328 kB
19.608 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
208.141 kB
59.349 kB
react-components
react-components: FluentProvider & webLightTheme
40.713 kB
13.509 kB
react-datepicker-compat
DatePicker Compat
211.441 kB
58.79 kB
react-dialog
Dialog (including children components)
89.925 kB
27.453 kB
react-divider
Divider
19.704 kB
7.38 kB
react-field
Field
21.036 kB
8.091 kB
react-image
Image
14.62 kB
5.869 kB
react-infobutton
InfoButton
129.602 kB
40.729 kB
react-infobutton
InfoLabel
133.328 kB
41.913 kB
react-input
Input
25.955 kB
8.81 kB
react-jsx-runtime
Classic Pragma
1.072 kB
544 B
react-jsx-runtime
JSX Dev Runtime
2.775 kB
1.29 kB
react-jsx-runtime
JSX Runtime
3.293 kB
1.529 kB
react-label
Label
13.036 kB
5.405 kB
react-link
Link
15.902 kB
6.506 kB
react-menu
Menu (including children components)
139.629 kB
43.01 kB
react-menu
Menu (including selectable components)
142.365 kB
43.541 kB
react-overflow
hooks only
12.594 kB
4.702 kB
react-persona
Persona
56.629 kB
17.505 kB
react-popover
Popover
118.701 kB
37.361 kB
react-portal
Portal
12.362 kB
4.543 kB
react-portal-compat
PortalCompatProvider
6.541 kB
2.227 kB
react-positioning
usePositioning
25.245 kB
9.141 kB
react-progress
ProgressBar
16.409 kB
6.58 kB
react-provider
FluentProvider
21.258 kB
7.937 kB
react-radio
Radio
29.318 kB
9.699 kB
react-radio
RadioGroup
14.344 kB
5.942 kB
react-select
Select
27.324 kB
9.773 kB
react-slider
Slider
36.849 kB
12.171 kB
react-spinbutton
SpinButton
35.53 kB
11.367 kB
react-spinner
Spinner
22.292 kB
8.113 kB
react-switch
Switch
31.885 kB
10.356 kB
react-table
DataGrid
156.416 kB
43.567 kB
react-table
Table (Primitives only)
42.463 kB
13.255 kB
react-table
Table as DataGrid
129.276 kB
34.739 kB
react-table
Table (Selection only)
74.56 kB
20.053 kB
react-table
Table (Sort only)
73.191 kB
19.649 kB
react-tags-preview
InteractionTag
13.85 kB
5.626 kB
react-tags-preview
Tag
29.604 kB
9.567 kB
react-tags-preview
TagGroup
72.493 kB
21.628 kB
react-text
Text - Default
15.644 kB
6.223 kB
react-text
Text - Wrappers
18.817 kB
6.546 kB
react-textarea
Textarea
30.005 kB
10.156 kB
react-theme
Single theme token import
69 B
89 B
react-theme
Teams: all themes
34.611 kB
7.295 kB
react-theme
Teams: Light theme
19.282 kB
5.486 kB
react-toast
Toast (including Toaster)
91.601 kB
27.394 kB
react-tooltip
Tooltip
51.154 kB
18.06 kB
react-utilities
SSRProvider
180 B
159 B
🤖 This report was generated against 3bf2de945735d2464b4aa4fcfccbf636a74651ef

@size-auditor
Copy link

size-auditor bot commented Sep 14, 2023

Asset size changes

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

Baseline commit: 3bf2de945735d2464b4aa4fcfccbf636a74651ef (build)

@fabricteam
Copy link
Collaborator

🕵 fluentuiv9 No visual regressions between this PR and main

@ling1726
Copy link
Member

Note: I haven't checked if sourcemaps still work after this change.

If it's a problem we can fix it later - more important we should fix failing builds for our users

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 636 637 5000
Breadcrumb mount 1655 1661 1000
Checkbox mount 1703 1696 5000
CheckboxBase mount 1472 1489 5000
ChoiceGroup mount 2884 2930 5000
ComboBox mount 645 641 1000
CommandBar mount 6218 6254 1000
ContextualMenu mount 11667 12286 1000
DefaultButton mount 748 755 5000
DetailsRow mount 2178 2203 5000
DetailsRowFast mount 2201 2205 5000
DetailsRowNoStyles mount 2011 2043 5000
Dialog mount 2661 2778 1000
DocumentCardTitle mount 219 231 1000
Dropdown mount 2002 1967 5000
FocusTrapZone mount 1123 1102 5000
FocusZone mount 1052 1072 5000
GroupedList mount 41053 40967 2
GroupedList virtual-rerender 19567 19438 2
GroupedList virtual-rerender-with-unmount 50308 50585 2
GroupedListV2 mount 229 234 2
GroupedListV2 virtual-rerender 210 208 2
GroupedListV2 virtual-rerender-with-unmount 233 231 2
IconButton mount 1122 1077 5000
Label mount 339 336 5000
Layer mount 2752 2749 5000
Link mount 394 384 5000
MenuButton mount 929 949 5000
MessageBar mount 21779 21670 5000
Nav mount 1955 1940 1000
OverflowSet mount 766 762 5000
Panel mount 1820 1766 1000
Persona mount 752 727 1000
Pivot mount 873 871 1000
PrimaryButton mount 860 833 5000
Rating mount 4630 4597 5000
SearchBox mount 936 917 5000
Shimmer mount 1895 1879 5000
Slider mount 1299 1298 5000
SpinButton mount 2851 2861 5000
Spinner mount 391 381 5000
SplitButton mount 1847 1840 5000
Stack mount 396 414 5000
StackWithIntrinsicChildren mount 852 853 5000
StackWithTextChildren mount 2606 2581 5000
SwatchColorPicker mount 6102 6185 5000
TagPicker mount 1484 1487 5000
Text mount 375 377 5000
TextField mount 951 927 5000
ThemeProvider mount 817 826 5000
ThemeProvider virtual-rerender 593 587 5000
ThemeProvider virtual-rerender-with-unmount 1294 1280 5000
Toggle mount 617 616 5000
buttonNative mount 185 194 5000

@layershifter layershifter merged commit bad33fa into microsoft:master Sep 14, 2023
@layershifter layershifter deleted the fix/strip-jsx-comments branch September 14, 2023 14:10
@@ -43,14 +43,19 @@ async function swcTransform(options: Options) {
outputPath,
});

// Strip @jsx comments, see https://github.com/microsoft/fluentui/issues/29126
Copy link

Choose a reason for hiding this comment

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

heads up that using @jsx in a comment without it actually being a jsx pragma directive is causing eslint-plugin-react to crash, which is something we need to fix. See jsx-eslint/eslint-plugin-react#3632.

Feel free in the meantime to avoid having @ and jsx next to each other in this comment if that ends up causing you any trouble :-)

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.

[Bug]: PCF build error pragma and pragmaFrag cannot be set when runtime is automatic.
4 participants