Future RFC - Asynchronously import hidden components#19609
Future RFC - Asynchronously import hidden components#19609czearing merged 7 commits intomicrosoft:masterfrom
Conversation
czearing
requested review from
behowell,
dzearing,
ecraig12345,
khmakoto and
smhigley
|
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 644d551:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: ae43b3623e0cff41c6dc55bc17de546d4e138d63 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 917 | 881 | 5000 | |
| BaseButton | mount | 905 | 921 | 5000 | |
| Breadcrumb | mount | 2682 | 2688 | 1000 | |
| ButtonNext | mount | 460 | 447 | 5000 | |
| Checkbox | mount | 1481 | 1506 | 5000 | |
| CheckboxBase | mount | 1259 | 1285 | 5000 | |
| ChoiceGroup | mount | 4784 | 4692 | 5000 | |
| ComboBox | mount | 998 | 971 | 1000 | |
| CommandBar | mount | 10343 | 10418 | 1000 | |
| ContextualMenu | mount | 6713 | 6657 | 1000 | |
| DefaultButton | mount | 1135 | 1147 | 5000 | |
| DetailsRow | mount | 3686 | 3699 | 5000 | |
| DetailsRowFast | mount | 3705 | 3777 | 5000 | |
| DetailsRowNoStyles | mount | 3579 | 3631 | 5000 | |
| Dialog | mount | 2465 | 2404 | 1000 | |
| DocumentCardTitle | mount | 147 | 146 | 1000 | |
| Dropdown | mount | 3213 | 3234 | 5000 | |
| FluentProviderNext | mount | 7479 | 7517 | 5000 | |
| FluentProviderWithTheme | mount | 358 | 316 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 95 | 100 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 479 | 483 | 10 | |
| FocusTrapZone | mount | 1776 | 1802 | 5000 | |
| FocusZone | mount | 1819 | 1808 | 5000 | |
| IconButton | mount | 1766 | 1761 | 5000 | |
| Label | mount | 333 | 350 | 5000 | |
| Layer | mount | 3050 | 3019 | 5000 | |
| Link | mount | 485 | 480 | 5000 | |
| MakeStyles | mount | 1816 | 1815 | 50000 | |
| MenuButton | mount | 1476 | 1485 | 5000 | |
| MessageBar | mount | 2023 | 2017 | 5000 | |
| Nav | mount | 3283 | 3227 | 1000 | |
| OverflowSet | mount | 1089 | 1095 | 5000 | |
| Panel | mount | 2365 | 2342 | 1000 | |
| Persona | mount | 826 | 840 | 1000 | |
| Pivot | mount | 1410 | 1409 | 1000 | |
| PrimaryButton | mount | 1277 | 1281 | 5000 | |
| Rating | mount | 7610 | 7739 | 5000 | |
| SearchBox | mount | 1330 | 1345 | 5000 | |
| Shimmer | mount | 2534 | 2533 | 5000 | |
| Slider | mount | 1953 | 1942 | 5000 | |
| SpinButton | mount | 4997 | 4930 | 5000 | |
| Spinner | mount | 425 | 414 | 5000 | |
| SplitButton | mount | 3208 | 3116 | 5000 | |
| Stack | mount | 489 | 505 | 5000 | |
| StackWithIntrinsicChildren | mount | 1564 | 1509 | 5000 | |
| StackWithTextChildren | mount | 4470 | 4518 | 5000 | |
| SwatchColorPicker | mount | 10255 | 10169 | 5000 | |
| Tabs | mount | 1433 | 1397 | 1000 | |
| TagPicker | mount | 2617 | 2574 | 5000 | |
| TeachingBubble | mount | 13391 | 13447 | 5000 | |
| Text | mount | 436 | 413 | 5000 | |
| TextField | mount | 1358 | 1389 | 5000 | |
| ThemeProvider | mount | 1192 | 1175 | 5000 | |
| ThemeProvider | virtual-rerender | 598 | 608 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1919 | 1891 | 5000 | |
| Toggle | mount | 792 | 806 | 5000 | |
| buttonNative | mount | 107 | 114 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ListMinimalPerf.default | 536 | 487 | 1.1:1 |
| LayoutMinimalPerf.default | 372 | 351 | 1.06:1 |
| ImageMinimalPerf.default | 364 | 346 | 1.05:1 |
| RefMinimalPerf.default | 243 | 231 | 1.05:1 |
| CardMinimalPerf.default | 572 | 549 | 1.04:1 |
| ChatWithPopoverPerf.default | 370 | 356 | 1.04:1 |
| GridMinimalPerf.default | 345 | 332 | 1.04:1 |
| HeaderMinimalPerf.default | 366 | 352 | 1.04:1 |
| TooltipMinimalPerf.default | 1024 | 989 | 1.04:1 |
| TreeWith60ListItems.default | 175 | 168 | 1.04:1 |
| AttachmentMinimalPerf.default | 153 | 148 | 1.03:1 |
| AvatarMinimalPerf.default | 189 | 184 | 1.03:1 |
| LabelMinimalPerf.default | 377 | 367 | 1.03:1 |
| MenuMinimalPerf.default | 841 | 819 | 1.03:1 |
| TextMinimalPerf.default | 343 | 334 | 1.03:1 |
| BoxMinimalPerf.default | 366 | 360 | 1.02:1 |
| ButtonSlotsPerf.default | 547 | 535 | 1.02:1 |
| CarouselMinimalPerf.default | 460 | 451 | 1.02:1 |
| CheckboxMinimalPerf.default | 2733 | 2674 | 1.02:1 |
| EmbedMinimalPerf.default | 4148 | 4074 | 1.02:1 |
| PortalMinimalPerf.default | 179 | 176 | 1.02:1 |
| RadioGroupMinimalPerf.default | 453 | 445 | 1.02:1 |
| SkeletonMinimalPerf.default | 354 | 347 | 1.02:1 |
| DatepickerMinimalPerf.default | 5407 | 5335 | 1.01:1 |
| DialogMinimalPerf.default | 761 | 755 | 1.01:1 |
| FlexMinimalPerf.default | 278 | 276 | 1.01:1 |
| FormMinimalPerf.default | 401 | 397 | 1.01:1 |
| HeaderSlotsPerf.default | 758 | 750 | 1.01:1 |
| ListCommonPerf.default | 629 | 621 | 1.01:1 |
| LoaderMinimalPerf.default | 694 | 689 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1721 | 1705 | 1.01:1 |
| ReactionMinimalPerf.default | 374 | 372 | 1.01:1 |
| StatusMinimalPerf.default | 664 | 657 | 1.01:1 |
| TableManyItemsPerf.default | 1899 | 1878 | 1.01:1 |
| TreeMinimalPerf.default | 791 | 784 | 1.01:1 |
| AccordionMinimalPerf.default | 139 | 139 | 1:1 |
| ChatMinimalPerf.default | 661 | 661 | 1:1 |
| DropdownMinimalPerf.default | 3088 | 3076 | 1:1 |
| PopupMinimalPerf.default | 598 | 596 | 1:1 |
| SegmentMinimalPerf.default | 347 | 346 | 1:1 |
| SplitButtonMinimalPerf.default | 4076 | 4078 | 1:1 |
| TextAreaMinimalPerf.default | 492 | 494 | 1:1 |
| ToolbarMinimalPerf.default | 926 | 922 | 1:1 |
| VideoMinimalPerf.default | 620 | 623 | 1:1 |
| ButtonMinimalPerf.default | 171 | 172 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1676 | 1690 | 0.99:1 |
| InputMinimalPerf.default | 1225 | 1240 | 0.99:1 |
| ItemLayoutMinimalPerf.default | 1183 | 1189 | 0.99:1 |
| MenuButtonMinimalPerf.default | 1651 | 1664 | 0.99:1 |
| RosterPerf.default | 1157 | 1164 | 0.99:1 |
| SliderMinimalPerf.default | 1541 | 1550 | 0.99:1 |
| TableMinimalPerf.default | 397 | 401 | 0.99:1 |
| CustomToolbarPrototype.default | 3803 | 3831 | 0.99:1 |
| AlertMinimalPerf.default | 266 | 272 | 0.98:1 |
| AnimationMinimalPerf.default | 399 | 409 | 0.98:1 |
| AttachmentSlotsPerf.default | 1070 | 1089 | 0.98:1 |
| ChatDuplicateMessagesPerf.default | 289 | 294 | 0.98:1 |
| DropdownManyItemsPerf.default | 671 | 682 | 0.98:1 |
| ListWith60ListItems.default | 636 | 646 | 0.98:1 |
| ProviderMinimalPerf.default | 966 | 988 | 0.98:1 |
| IconMinimalPerf.default | 588 | 601 | 0.98:1 |
| DividerMinimalPerf.default | 339 | 348 | 0.97:1 |
| ListNestedPerf.default | 532 | 547 | 0.97:1 |
|
|
||
| Components such as `Tooltip` are split into 2 parts: | ||
|
|
||
| 1. A synchronous tiny component |
There was a problem hiding this comment.
Can we get info about the actual size of the "tiny" part for something like Tooltip or Menu?
There was a problem hiding this comment.
Would dynamic imports might be most useful in the icon case to only bring in the component with the SVG when used?
There was a problem hiding this comment.
tiny part is the trigger code required to "start" showing the menu/tooltip. likely less< 1k.
| ```jsx | ||
| <> | ||
| {state.children} | ||
| {state.shouldRenderTooltip && (...)} |
There was a problem hiding this comment.
can you please add to the example how we would apply aria-label / labelledby while the full tooltip component is not loaded?
There was a problem hiding this comment.
I'm not sure it would ever work properly to use a tooltip as the target of aria-labelledby, regardless of whether it's synchronously or async loaded (unless I'm misunderstanding what you mean). @smhigley could confirm.
There was a problem hiding this comment.
I mean the fucntionality of applying label/description based on triggerAriaAttribute prop on the trigger:
how would that work when there is only the minimal part loaded?
|
|
||
| #### Cons | ||
|
|
||
| - Partners who use old bundlers would potentially be affected. |
There was a problem hiding this comment.
If a user does not use something like create-react-app and uses a minimal webpack config, would that break their app until they add the approriate code splitting plugin ?
There was a problem hiding this comment.
Webpack should handle that automatically (no plugins needed) since import() is standard syntax now.
There was a problem hiding this comment.
This is the semi-relevant section in the docs. It's not super explicit about this but from what's there and my experience I'm still pretty sure it will "just work." SplitChunksPlugin or whatever is only needed if you want to have highly customized splitting behavior. https://webpack.js.org/guides/code-splitting/#dynamic-imports
khmakoto
left a comment
khmakoto
left a comment
There was a problem hiding this comment.
I don't know if this is an issue or not, but are there any concerns in using async imports on SSR scenarios?
Good question. Async imports work as long as you are using node 14 with modules however |
| <> | ||
| {state.children} | ||
| {state.shouldRenderTooltip && ( | ||
| <React.Suspense fallback={null}> |
There was a problem hiding this comment.
- What happens when this fails to load/loads after
.shouldRenderTooltipbecomestrue? Customers will see nothing? - SSR is not supported by
.Suspense➡ we can't rely on it currently
Please check for inspiration: https://github.com/theKashey/use-sidecar
I haven't checked how it works, but it's very close to our goal I guess
There was a problem hiding this comment.
If a javascript file fails to load on web app startup or on an async on demand bundle, this is essentially out of scope of fluent ui. Some webpack partners override the webpack async loading logic to have retries and fallbacks and ultimately a BSOD equivalent.
|
|
||
| ## Problem statement | ||
|
|
||
| We have several scenarios where async imports would be beneficial for minimizing bundle size impact on our partners apps, especially for initial page load. |
There was a problem hiding this comment.
Users with limited bandwidth or where the last mile is distant from origin, the cost of each JS download can be mostly the request resolution and server connection. Once the data starts downloading, the payload size doesn't cost as much relative to the connection. In other words, if there are lots of small requests it can be more expensive.
This is typically solved with CDNs or edge caching.
- What the experience would be with devs wanting to use a CDN?
- Would each dynamic import be able to be resolved to a CDN address?
- Does having lots of code splits mean that there are more things being invalidated as fluent changes?
There was a problem hiding this comment.
SplitChunksPlugin has options to help configure this on the consumer side, including without any knowledge of paths/filenames. Most nontrivial apps will already have async imports and multiple bundles, and probably have SplitChunksPlugin or a similar solution set up to customize it. So I don't think this is something we need to worry about directly.
|
Meeting notes:
|
|
I think closing this prematurely without a clear plan of next steps should be clarified a little. Essentially, designing for sync/async splits can change the design of a component significantly. You need to separate your logic into the "synchronous" inexpensive part from the "async" expensive part and ask/answer questions about this. It doesn't have to be complicated, and it doesn't have to apply to every component. But we're working on tooltip and menu now, so really this should be considered as part of the design in those components. But history has shown here that ignoring bundle size leads to customers being frustrated, and we've done this sort of thing in the past. I think in the past there weren't standard practices involved here. But there are now. So we should design for them. IRT meeting notes:
SHysom is a great resource here if you have questions on bundling/splitting in ooui. But I think rather than assigning a dev who can help investigate specific major partners on their bundle strat, we could have something more outcome oriented. E.g. Importing Menu or Tooltip get split automatically, so your initial page render skips on the 30k of popper/focuszone/portal/etc code. This might be done in multiple ways if async imports has some sort of snag. This RFC originated from conversations with him and others on his team working on Ribbon perf. It is really important that we avoid bundle penalties for using Tooltips and Menus. They are used frequently in Ribbon and other Office scenarios. A couple years ago, one dev was evaluating Menus and worked closely with KChau on how to defer the menu loading code because it brought in a swarm of dependencies. It was painful because we didn't have tools and Ken wrote a webpack plugin specifically to support this scenario because there wasn't a great way on the app consumer side to know exactly where/how to split. It wasn't considered in the design. We have an opportunity with converged components to design these limited scenarios correctly so that bundle size isn't penalized. Ignoring it will likely repeat history.
No more than any other async import in the bundle. Apps which support offline have a manifest of resources to be downloaded, so that they CAN load their app offline.
You could certainly have an import like "useSyncTooltip" which synchronously includes both things. Partners could import/call it in their app code (so that it shows up in the app bundle and dependencies aren't tree shaken.) Then when the async imports in Tooltip kick off they resolve to things in the app bundle. But I would avoid this until there's a real use case.
I'm not really sure it's an issue for things which don't render on initial page load. SSR would only render the visible things and avoid calling lifetime operations. But honestly Tooltip/Menu devs should try rendering those components in SSR and figure out problems/solutions; we need devs to understand SSR scenarios.
Yep Ken did this. Nobody discovers it. The default should do the right thing with no config. You should have to work hard to do the weird thing like synchronously rendering tooltip. While maybe the RFC doesn't need to be set in stone right now, I would strongly urge having the Menu/Tooltip devs drive a solution for the bundle problem that this is addressing, and really take over the RFC and drive it to completion. I'm afraid opening an issue without a driver will go to graveyard and lead to "let's just make everything synchronous, and let the app dev deal with it because that's easy." We all need great empathy towards minimizing bundle bloat, and async imports are low hanging fruit imo. |
|
@dzearing Ah okay, I wasn't aware that this was immediately impactful for Menu and Tooltip (I thought the motivating scenario was specifically the tooltip on Slider, which is definitely less urgent). cc @ling1726 and @behowell The major blocker here is lack of support for async imports in Node without newer versions and/or experimental flags, causing issues in SSR and maybe tests (though tests have to use commonjs still for related reasons). Lack of support for React lazy/suspense in SSR is also an issue. Unless we can find a way around those things, we probably can't use async imports in our code directly. So then the question becomes what we can do instead--there were some ideas, but all of them need further investigation.
Alternatively, it might be possible to say (and document) that SSR is an edge case where it's okay to require extra config, and provide or recommend a babel plugin or something to convert all async imports to sync. This would probably be okay with our major partner teams (since AFAIK none of them use SSR?) but seems a bit dismissive of the larger community. (Another quick note on conversion plugins: writing all imports as sync and using a plugin to selectively convert to async is much more complicated and may not be feasible.) Even if we find a way around the SSR issues and decide to start using async imports in our code, there are still quite a few outstanding questions about the implications in different scenarios:
Basically we need a lot more investigation, including demos of various approaches, and the consensus among those who were in the meeting this morning was that we just don't have time for that right now. However that consensus was not considering the general issues you raised about menu and tooltip triggers--we didn't discuss that at all since we weren't aware that was an immediate concern. |
|
If in your typescript code you say: export function getFoo() {
import("./Foo").then((foo) => {
console.log(foo);
});
}...and you compile to commonjs modules, it produces this: "use strict";
exports.__esModule = true;
exports.getFoo = void 0;
function getFoo() {
Promise.resolve().then(function () { return require("./Foo"); }).then(function (foo) {
console.log(foo);
});
}
exports.getFoo = getFoo;Which I believe is compatible with node 12 and even older (wherever Promises were introduced.) |
|
@dzearing Maybe a silly question (I'm not familiar with this area) but is it expected in general that SSR would have to use the commonjs version of the code? |
|
pretty likely; I think when you import a
One thing I'm not totally sure on however is if it infers the module default if the initial resolution was due to |
|
Also, I assume this edge case isn't an issue for older versions that don't support async imports (v12?) because likely they would also not try to load the entry from |
|
One idea that comes to mind is that this might be a good candidate for a v10 change. My thoughts are that we're mid releasing beta and a stable 9.0.0 soon after and async, while valuable, is likely to be a significant departure from the current architecture. We have many good things to provide in v9 as it stands and should continue with shipping that, but we should also continue this async proposal. I would second @dzearing's comments about bundle size issues for importing things that are not used on first load. It was one of the things we discussed in the very early days of convergence as both libs suffered from issues with sync interdependencies like this. All said, this topic deserves a proper amount of time to think through it and test. I don't see that fitting on the 9.0 runway, especially given that we're currently cutting items to make the beta/major targets. But, why don't we continue this conversation and give ourselves some breathing room by considering it a v10 proposal? |
|
For RFCs that are planning to be re-visited, is there an rfc folder for "v10" that this can be merged into (where all the v10 plans are), so that there's an easy way to come back to it? |
|
We do have a |
…eat/async-import-hidden-elements
czearing
changed the title
|
I'm reopening the pull request to move the RFC into the future folder so it can be re-visited. |
khmakoto
left a comment
khmakoto
left a comment
There was a problem hiding this comment.
Approving now that this is being moved into the future folder, although maybe we should make a note about how we are continuing this conversation later at the beginning or end of the RFC.
"Previously discussed concerns".
There was a problem hiding this comment.
I just pushed an update adding myself to the list of drivers (since you may not be around when this next comes up), switching the comparison to gzip sizes (this is preferred because it's what would actually be downloaded by end users' computers), and clarifying which section of the proposal is the original part that hasn't yet been updated from PR discussion. With that it LGTM. Thanks for working on this!
* Adding Future RFC * Quick fix * PR feedback * Moving the async import RFC to the future folder * Adding sections for "Continuing the discussion" and "Previously discussed concerns". * minor updates Co-authored-by: Elizabeth Craig <elcraig@microsoft.com>
11 participants
This RFC proposes the idea of asynchronously importing hidden components such as
TooltipandContextualMenuto assist in bundle size related issues.