iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
84 stars 23 forks source link

feat!: Add `NonIdealState` and deprecate `ErrorPage` #924

Closed mayank99 closed 1 year ago

mayank99 commented 1 year ago

Removed internal cjs imports for @itwin/itwinui-illustrations-react and replaced with module imports. This still results in all illustrations being bundled, even if only only one of them is used. After failed attempt at dynamic imports, I've decided to deprecate ErrorPage and create a new NonIdealState that inverts the control back to user who can choose to code split however they want.

Old PR description about dynamic imports Removed internal cjs imports for `@itwin/itwinui-illustrations-react` and replaced with dynamic imports combined with `React.lazy` and `Suspense`. This will massively lower bundle sizes for everyone while avoiding CJS/ESM interop issues. --- I had to create duplicate files inside our projects so that I could re-export module imports as default exports. This is because it is not possible to use a module import with `React.lazy`. See docs: https://beta.reactjs.org/apis/react/lazy and https://reactjs.org/docs/code-splitting.html#reactlazy --- On slow networks, a spinner will be shown while the illustration loads. On fast networks, this should not be very noticeable (especially with SSR). https://user-images.githubusercontent.com/9084735/200956422-265f8d70-b507-4e98-b4bc-8788ed090e62.mov
veekeys commented 1 year ago

Just an idea to check SSR react support for lazy and suspense. It was introduced in 16.6, but we need to be sure that ssr also got support before our minimum version. https://reactjs.org/blog/2018/10/23/react-v-16-6.html

elephantcatdog commented 1 year ago

Have you considered placing a minimum time on the loading spinner so that it always displays for 1 second or so versus the spinner blinking when the network is fast? I think this is something I've done on past teams.

This link explains it well: https://blog.bitsrc.io/a-brief-history-of-flickering-spinners-c9eecd6053

mayank99 commented 1 year ago

Just tested this in nextjs and while it appears correctly on the page, I get this error in dev server on first static render:

Unhandled Runtime Error: This Suspense boundary received an update before it finished hydrating.
This caused the boundary to switch to client rendering. The usual way to fix this is to wrap the original update in startTransition.

This seems like a common problem in nextjs (see https://github.com/vercel/next.js/issues/35728). I tried wrapping in startTransition (see code below) but could not get it to work. I believe that only works with react state updates.

See code ```tsx const dynamicImports = { '401': () => import('../utils/illustrations/Svg401'), '403': () => import('../utils/illustrations/Svg403'), '404': () => import('../utils/illustrations/Svg404'), '500': () => import('../utils/illustrations/Svg500'), '502': () => import('../utils/illustrations/Svg502'), '503': () => import('../utils/illustrations/Svg503'), error: () => import('../utils/illustrations/SvgError'), redirect: () => import('../utils/illustrations/SvgRedirect'), timedOut: () => import('../utils/illustrations/SvgTimedOut'), }; const startTransition = React.startTransition || ((callback) => callback()); const load = async (errorType: keyof typeof dynamicImports) => { const lazyComponentPromise = dynamicImports[errorType](); if (typeof window === 'undefined') { return (await lazyComponentPromise) as { default: React.ComponentType; }; } return (await new Promise((resolve) => { startTransition(() => { lazyComponentPromise.then((module) => { resolve(module); }); }); })) as { default: React.ComponentType }; }; const Svg401 = React.lazy(() => load('401')); const Svg403 = React.lazy(() => load('403')); const Svg404 = React.lazy(() => load('404')); const Svg500 = React.lazy(() => load('500')); const Svg502 = React.lazy(() => load('502')); const Svg503 = React.lazy(() => load('503')); const SvgError = React.lazy(() => load('error')); const SvgRedirect = React.lazy(() => load('redirect')); const SvgTimedOut = React.lazy(() => load('timedOut')); ```

I don't think this is worth investigating so I'm going to abandon this effort and just using module imports (ESM vs CJS should be fixed in #845 and https://github.com/iTwin/iTwinUI-icons/pull/49).

The downside is that all illustrations will be part of the client bundle, although it's not that different from our current situation (we import all illustrations in ErrorPage even if only one is used). To solve this, I've made a new component called NonIdealState which uses "inversion of control", making the user in charge of providing the illustration. This way, the user can import the single svg that they are actually using. And if the user doesn't import ErrorPage, then it should get treeshaken out and the illustrations will not be imported. Additionally, the user will also be able to use their framework's suggested approach for code splitting (e.g. next/dynamic) instead of us deciding to hardcode React.lazy for them.

I'm also deprecating the ErrorPage component since NonIdealState is clearly better. Not removing it yet because many of our users are relying on this, especially the part where it automatically maps error codes to svgs (see discussion in #715).

Note that this uses css changes from https://github.com/iTwin/iTwinUI/pull/806.