Add forwardRef wrapper to clean up component types #19812
Conversation
behowell
requested review from
czearing,
ecraig12345 and
a team
as code owners
|
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 31f0994:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 5107c383813b6ca883958375d714de18ead75e06 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Panel | mount | 2188 | 1299 | 1000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 884 | 846 | 5000 | |
| BaseButton | mount | 844 | 838 | 5000 | |
| Breadcrumb | mount | 2480 | 2473 | 1000 | |
| ButtonNext | mount | 419 | 418 | 5000 | |
| Checkbox | mount | 1420 | 1410 | 5000 | |
| CheckboxBase | mount | 1192 | 1155 | 5000 | |
| ChoiceGroup | mount | 4418 | 4400 | 5000 | |
| ComboBox | mount | 943 | 897 | 1000 | |
| CommandBar | mount | 9700 | 9601 | 1000 | |
| ContextualMenu | mount | 5987 | 6146 | 1000 | |
| DefaultButton | mount | 1086 | 1043 | 5000 | |
| DetailsRow | mount | 3495 | 3493 | 5000 | |
| DetailsRowFast | mount | 3548 | 3455 | 5000 | |
| DetailsRowNoStyles | mount | 3297 | 3302 | 5000 | |
| Dialog | mount | 2319 | 2326 | 1000 | |
| DocumentCardTitle | mount | 135 | 127 | 1000 | |
| Dropdown | mount | 3069 | 3026 | 5000 | |
| FluentProviderNext | mount | 7068 | 7128 | 5000 | |
| FluentProviderWithTheme | mount | 327 | 327 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 88 | 92 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 438 | 445 | 10 | |
| FocusTrapZone | mount | 1658 | 1668 | 5000 | |
| FocusZone | mount | 1739 | 1734 | 5000 | |
| IconButton | mount | 1633 | 1639 | 5000 | |
| Label | mount | 318 | 314 | 5000 | |
| Layer | mount | 2827 | 2814 | 5000 | |
| Link | mount | 435 | 442 | 5000 | |
| MakeStyles | mount | 1734 | 1766 | 50000 | |
| MenuButton | mount | 1378 | 1362 | 5000 | |
| MessageBar | mount | 1897 | 1909 | 5000 | |
| Nav | mount | 3049 | 3058 | 1000 | |
| OverflowSet | mount | 1038 | 1071 | 5000 | |
| Panel | mount | 2188 | 1299 | 1000 | Possible regression |
| Persona | mount | 800 | 777 | 1000 | |
| Pivot | mount | 1356 | 1347 | 1000 | |
| PrimaryButton | mount | 1230 | 1207 | 5000 | |
| Rating | mount | 7183 | 7189 | 5000 | |
| SearchBox | mount | 1235 | 1216 | 5000 | |
| Shimmer | mount | 2372 | 2403 | 5000 | |
| Slider | mount | 1821 | 1855 | 5000 | |
| SpinButton | mount | 4680 | 4748 | 5000 | |
| Spinner | mount | 404 | 383 | 5000 | |
| SplitButton | mount | 2960 | 2970 | 5000 | |
| Stack | mount | 471 | 473 | 5000 | |
| StackWithIntrinsicChildren | mount | 1465 | 1464 | 5000 | |
| StackWithTextChildren | mount | 4244 | 4292 | 5000 | |
| SwatchColorPicker | mount | 9640 | 9718 | 5000 | |
| Tabs | mount | 1334 | 1326 | 1000 | |
| TagPicker | mount | 2432 | 2499 | 5000 | |
| TeachingBubble | mount | 12575 | 12494 | 5000 | |
| Text | mount | 398 | 390 | 5000 | |
| TextField | mount | 1309 | 1277 | 5000 | |
| ThemeProvider | mount | 1122 | 1141 | 5000 | |
| ThemeProvider | virtual-rerender | 572 | 570 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1769 | 1775 | 5000 | |
| Toggle | mount | 738 | 761 | 5000 | |
| buttonNative | mount | 110 | 114 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ChatDuplicateMessagesPerf.default | 277 | 254 | 1.09:1 |
| IconMinimalPerf.default | 614 | 562 | 1.09:1 |
| AvatarMinimalPerf.default | 187 | 177 | 1.06:1 |
| TextAreaMinimalPerf.default | 484 | 458 | 1.06:1 |
| GridMinimalPerf.default | 319 | 304 | 1.05:1 |
| AttachmentMinimalPerf.default | 145 | 139 | 1.04:1 |
| CarouselMinimalPerf.default | 431 | 416 | 1.04:1 |
| ImageMinimalPerf.default | 360 | 345 | 1.04:1 |
| LabelMinimalPerf.default | 363 | 350 | 1.04:1 |
| PopupMinimalPerf.default | 568 | 545 | 1.04:1 |
| PortalMinimalPerf.default | 173 | 167 | 1.04:1 |
| CardMinimalPerf.default | 520 | 505 | 1.03:1 |
| ChatMinimalPerf.default | 617 | 597 | 1.03:1 |
| DropdownManyItemsPerf.default | 629 | 613 | 1.03:1 |
| FlexMinimalPerf.default | 274 | 266 | 1.03:1 |
| SegmentMinimalPerf.default | 325 | 316 | 1.03:1 |
| SkeletonMinimalPerf.default | 330 | 319 | 1.03:1 |
| AnimationMinimalPerf.default | 390 | 381 | 1.02:1 |
| FormMinimalPerf.default | 379 | 370 | 1.02:1 |
| HeaderMinimalPerf.default | 341 | 334 | 1.02:1 |
| RadioGroupMinimalPerf.default | 412 | 405 | 1.02:1 |
| SliderMinimalPerf.default | 1509 | 1475 | 1.02:1 |
| ToolbarMinimalPerf.default | 887 | 870 | 1.02:1 |
| VideoMinimalPerf.default | 579 | 568 | 1.02:1 |
| AttachmentSlotsPerf.default | 1013 | 1000 | 1.01:1 |
| ButtonSlotsPerf.default | 520 | 513 | 1.01:1 |
| DialogMinimalPerf.default | 721 | 715 | 1.01:1 |
| DropdownMinimalPerf.default | 2966 | 2941 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1124 | 1108 | 1.01:1 |
| ListWith60ListItems.default | 593 | 588 | 1.01:1 |
| LoaderMinimalPerf.default | 651 | 643 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1529 | 1517 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1567 | 1549 | 1.01:1 |
| ReactionMinimalPerf.default | 348 | 343 | 1.01:1 |
| TableManyItemsPerf.default | 1785 | 1771 | 1.01:1 |
| TextMinimalPerf.default | 330 | 327 | 1.01:1 |
| TooltipMinimalPerf.default | 955 | 946 | 1.01:1 |
| AccordionMinimalPerf.default | 139 | 139 | 1:1 |
| CheckboxMinimalPerf.default | 2533 | 2537 | 1:1 |
| EmbedMinimalPerf.default | 3886 | 3897 | 1:1 |
| LayoutMinimalPerf.default | 336 | 337 | 1:1 |
| ListCommonPerf.default | 570 | 571 | 1:1 |
| MenuMinimalPerf.default | 776 | 776 | 1:1 |
| RosterPerf.default | 1093 | 1095 | 1:1 |
| SplitButtonMinimalPerf.default | 3871 | 3867 | 1:1 |
| StatusMinimalPerf.default | 643 | 640 | 1:1 |
| TableMinimalPerf.default | 373 | 374 | 1:1 |
| CustomToolbarPrototype.default | 3710 | 3700 | 1:1 |
| TreeMinimalPerf.default | 729 | 730 | 1:1 |
| BoxMinimalPerf.default | 312 | 316 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1606 | 1617 | 0.99:1 |
| DatepickerMinimalPerf.default | 5115 | 5157 | 0.99:1 |
| ListNestedPerf.default | 498 | 503 | 0.99:1 |
| ProviderMinimalPerf.default | 939 | 950 | 0.99:1 |
| TreeWith60ListItems.default | 167 | 169 | 0.99:1 |
| AlertMinimalPerf.default | 249 | 254 | 0.98:1 |
| DividerMinimalPerf.default | 320 | 326 | 0.98:1 |
| InputMinimalPerf.default | 1174 | 1192 | 0.98:1 |
| HeaderSlotsPerf.default | 694 | 714 | 0.97:1 |
| ListMinimalPerf.default | 468 | 481 | 0.97:1 |
| ButtonMinimalPerf.default | 147 | 153 | 0.96:1 |
| ChatWithPopoverPerf.default | 327 | 339 | 0.96:1 |
| RefMinimalPerf.default | 212 | 222 | 0.95:1 |
| * const Example = forwardRef<ExampleProps & React.RefAttributes<HTMLElement>>((props, ref) => { ... }); | ||
| * ``` | ||
| * | ||
| * @returns the same as `React.forwardRef`, re-typed to `React.FunctionCompoonent<Props>`. |
There was a problem hiding this comment.
nit:
| * @returns the same as `React.forwardRef`, re-typed to `React.FunctionCompoonent<Props>`. | |
| * @returns the same as `React.forwardRef`, re-typed to `React.FunctionComponent<Props>`. |
| @@ -70,6 +70,11 @@ export type IntrinsicShorthandProps< | |||
| */ | |||
| export type IsSingleton<T extends string> = { [K in T]: Exclude<T, K> extends never ? true : false }[T]; | |||
|
|
|||
| /** | |||
| * Extends a props object with a LegagyRef, to be a normal Ref | |||
There was a problem hiding this comment.
nit:
| * Extends a props object with a LegagyRef, to be a normal Ref | |
| * Extends a props object with a LegacyRef, to be a normal Ref |
| * Expects the Props type to already have a `ref` attribute (which is the case if using `IntrinsicShorthandProps`). | ||
| * If not, add the ref attribute using `React.RefAttributes`: | ||
| * ``` | ||
| * const Example = forwardRef<ExampleProps & React.RefAttributes<HTMLElement>>((props, ref) => { ... }); |
There was a problem hiding this comment.
Do you want to encourage callers to have ExampleProps be intersected with the ref attributes rather than doing it within the forwardRef call?
There was a problem hiding this comment.
I'm not 100% sure I understand, but is the suggestion that this function should add a ref attribute if it doesn't exist?
If the Props param doesn't have a ref attribute, this function can't add it because it doesn't know what type the ref should be: Ref<HTMLDivElement> or Ref<HTMLSpanElement>, etc. (It's not correct to just use Ref<HTMLElement>, since that would allow you to assign e.g. Ref<HTMLButtonElement> to a component whose root type is <input>.)
The Props param to this function is required to have a ref prop, and this example was demonstrating how to add it if it's not already there.
|
Are we sure that we want to use |
Interesting, didn't realize that. (This link is to 16 but it's similar in latest.) I guess we could either have the wrapper return |
I don't think anything is wrong with using the default types for |
Using the return value without an explicit type results in unhelpful expansion of types. It's not the end of the world, but I'd still strongly prefer to have an explicit type of some kind. |
The main problem is that fowardRef's default types internally use |
I just started trying that out locally and it breaks down for Link. 😢 Best thing I can come up with is a cast. export const Link: React.ForwardRefExoticComponent<LinkProps> = React.forwardRef((props, ref) => {
const state = useLink(props, ref as React.Ref<HTMLAnchorElement | HTMLButtonElement>); |
|
Hmm actually it breaks down in the same way in other places, I just didn't see it at first due to outdated types cached in VS Code's memory or something. export const Accordion: React.ForwardRefExoticComponent<AccordionProps> = React.forwardRef((props, ref) => {
const state = useAccordion(props, ref); // line 11 |
|
Please do not merge this till Monday, I will leave more detailed comment. TLDR; this function will not be annotated with |
… purely a re-typing of React.forwardRef
…lotprops-forwardref
👍🏻 I'll wait. I did just change it to be simply re-exporting forwardRef with a different type; I'm not sure if that changes the issue you're referring to (I don't fully understand the problem). |
Rather than re-exporting with a different type, could we just add a helper type and use that in the declarations? like |
|
Tested on webpack@5.21.2 & terser@5.5.1. BackgroundWebpack does not do dead-code elimination, it does only module concatenation and it's enabled by default 🎉 Minification and dead code elimination is done by Terser. It's important to have concatenated modules as then Terser operates in a flattened scope. Check examples below 👇 Sample input// exports.js
export const foo = "foo";
export const bar = "bar";// main.js
import { foo, bar } from "./exports";
console.log(foo);Webpack's output with `optimization: { concatenateModules: false }`/******/ (() => {
// webpackBootstrap
/******/ "use strict";
/******/ var _a__WEBPACK_IMPORTED_MODULE_0__,
__webpack_modules__ = {
/***/ 85: /***/ (
__unused_webpack_module,
__webpack_exports__,
__webpack_require__
) => {
/* harmony export */ __webpack_require__.d(__webpack_exports__, {
/* harmony export */ R: () => /* binding */ foo
/* harmony export */
});
/* unused harmony export bar */
const foo = "foo";
}
/***/
/******/
},
__webpack_module_cache__ = {}; /******/ /******/ // The module cache // The require function
/************************************************************************/
/******/ /******/ /******/ function __webpack_require__(moduleId) {
/******/ // Check if module is in cache
/******/ if (__webpack_module_cache__[moduleId])
/******/ return __webpack_module_cache__[moduleId].exports; // Create a new module (and put it into the cache)
/******/
/******/ /******/ var module = (__webpack_module_cache__[moduleId] = {
/******/ // no module.id needed
/******/ // no module.loaded needed
/******/ exports: {}
/******/
}); /******/ /******/ // Execute the module function // Return the exports of the module
/******/
/******/ /******/ /******/ return (
__webpack_modules__[moduleId](
module,
module.exports,
__webpack_require__
),
module.exports
);
/******/
} /******/ /* webpack/runtime/define property getters */ // define getter functions for harmony exports
/******/
/************************************************************************/
/******/ /******/ /******/ (__webpack_require__.d = (exports, definition) => {
/******/ /******/ for (var key in definition)
__webpack_require__.o(definition, key) &&
!__webpack_require__.o(exports, key) &&
/******/ Object.defineProperty(exports, key, {
enumerable: !0,
get: definition[key]
});
/******/
/******/
}),
/******/ (__webpack_require__.o = (obj, prop) =>
Object.prototype.hasOwnProperty.call(
obj,
prop
)) /************************************************************************/ /******/,
/******/ (_a__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(85)),
console.log(_a__WEBPACK_IMPORTED_MODULE_0__ /* .foo */.R);
})();
/******/Webpack's output with enabled module concatenation/******/ (() => {
// webpackBootstrap
/******/ "use strict";
// CONCATENATED MODULE: ./src/main.js
console.log("foo");
})();
/******/Why do we care at all about module concatenation?React is CommonJS module and cannot use module concatenation as it works only for ESM 🙃 So after using React's API we will get something different: // exports.js
+import * as React from "react";
-export const foo = "foo";
-export const bar = "bar";
+export const foo = React.forwardRef("foo");
+export const bar = React.forwardRef("bar");Output from Webpack: /************************************************************************/ (() => {
// EXTERNAL MODULE: ./node_modules/react/index.js
var react = __webpack_require__(294);
// CONCATENATED MODULE: ./src/exports.js
const foo = react.forwardRef("foo");
react.forwardRef("bar");
// CONCATENATED MODULE: ./src/main.js
console.log(foo);
})();⚠ You could notice that The output will be the same even if we use imports from separate files (importing a module is enough to create issues): // ⚠ exports.js was split to two files
import { foo } from "./foo";
import { bar } from "./bar";
console.log(foo);In real life this could be conditional import based on env variables or a flag. This happens because This could be solved by // bar.js
import * as React from "react";
-export const bar = React.forwardRef("bar");
+export const bar = /*#__PURE__*/ React.forwardRef("bar");⬇⬇⬇ /************************************************************************/ (() => {
// CONCATENATED MODULE: ./src/foo.js
const foo = __webpack_require__(294).forwardRef("foo");
// CONCATENATED MODULE: ./src/main.js
console.log(foo);
})();That's why we have this Babel plugin in our pipeline for vNext components, #18037. How this PR can cause problems?Babel plugin annotates only calls of // ref.js
import * as React from "react";
export const forwardRef = fn => {
return /*#__PURE__*/ React.forwardRef(fn);
};// EXTERNAL MODULE: ./node_modules/react/index.js
var react = __webpack_require__(294);
// CONCATENATED MODULE: ./src/ref.js
const forwardRef = fn => /*#__PURE__*/ react.forwardRef(fn),
foo = forwardRef("foo");
forwardRef("bar");
// CONCATENATED MODULE: ./src/main.js
console.log(foo);👆 The issue is still present for the latest change in this PR (a707aa9). // ref.js
import * as React from "react";
export const forwardRef = /*#__PURE__*/ React.forwardRef;/******/ (() => {
// webpackBootstrap
/******/ "use strict";
// CONCATENATED MODULE: external "react"
const forwardRef = /*#__PURE__*/ react.forwardRef,
foo = forwardRef("foo");
forwardRef("bar");
// CONCATENATED MODULE: ./src/main.js
console.log(foo);
})();
/******/Is it solvable?Yes ✅ We just need to use // foo.js
import { forwardRef } from "./ref";
-export const foo = forwardRef("foo");
+export const foo = /*#__PURE__*/ forwardRef("foo");Then output becomes great again: /******/ (() => {
// webpackBootstrap
/******/ "use strict";
// CONCATENATED MODULE: external "react"
const ref_forwardRef = react.forwardRef;
// CONCATENATED MODULE: ./src/main.js
console.log(/*#__PURE__*/ ref_forwardRef("foo"));
})();
/******/💡 We can still do it in automated way via
|
|
@layershifter Thanks for the explanation!
I would add that this isn't necessarily just API Extractor. It's at least two different reasons:
I had been looking at this some on Friday, so especially since Ben is out now (sooner than expected), I may make another PR with a similar goal but a slightly different approach. |
|
Is this superseded by #19923? If yes, can this be closed? |
Yes, thanks 👍 |
Pull request checklist
$ yarn changeDescription of changes
This is a follow-up from PR #19793. It adds a wrapper for
forwardReffor use by our components. This has the following benefits:IntrinsicShorthandProps. This reduces the chance of human error/laziness in getting the wrong type for therefprop.React.PropsWithoutRef, whichReact.forwardRefuses. This type caused intrinsic element types to be exploded inside the .api.md file (listing every HTML attribute individually).React.FunctionComponent<ComponentProps>instead of the current mix of React.ForwardRefExoticComponent, React.FunctionComponent, etc.