[code-infra] Setup error message minification #1463
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Janpot
changed the title
Janpot
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
github-actions
bot
removed
the
PR: out-of-date
commit: |
Bundle size reportTotal Size Change: @base-ui-components/react/separator parsed: Show 29 more bundle changes@base-ui-components/react/fieldset parsed: |
github-actions
bot
added
the
PR: out-of-date
|
Since the bundle size checker is running, we can continue with this. The size saving aren't enormous (-0.80% gzipped for the whole library), but they might get slightly more significant as we create more components that throw more errors. I tested an alternative approach, but it's much worse: #2050 One additional thing we need here is for the build to fail if error messages haven't been extracted. |
It would also enable more verbose error messages for a better DX. in case you were holding back atm 🙂. We can keep this on hold for a bit longer as well as far as I'm concerned to let the lib get a little more stable.
Currently CI should fail by checking changes, do you want to build this into the build step directly? |
No, that's fine :) There are size savings already, so we can move forward with this. I'd like to merge #2049 first, though, so the messages are consistent.
OK, that's enough. |
babel.config.js
Outdated
| }, | ||
| }, | ||
| ], | ||
| 'babel-plugin-optimize-clsx', |
github-actions
bot
removed
the
PR: out-of-date
Bundle size report
|
| * ... | ||
| * @param {number} code | ||
| */ | ||
| export default function formatErrorMessage(code: number, ...args: string[]): string { |
There was a problem hiding this comment.
Shouldn't this be in @base-ui-components/utils and then imported and used in react package.
| 1. There hasn't been an update to the semantic meaning of the error message. Error codes need to outlive Base UI versions, so the same code must mean the same thing across versions. | ||
| 2. There hasn't been a change in parameters, no added and no removed. | ||
|
|
||
| In both of those cases, always create a new error code lline in `./src/error-codes.json`. |
There was a problem hiding this comment.
| In both of those cases, always create a new error code lline in `./src/error-codes.json`. | |
| In both of those cases, always create a new error code line in `./src/error-codes.json`. |
| const errorCodesPath = new URL(import.meta.resolve('./docs/public/static/error-codes.json')) | ||
| .pathname; | ||
| const missingError = process.env.MUI_EXTRACT_ERROR_CODES === 'true' ? 'write' : 'annotate'; | ||
| const errorCodesPath = new URL(import.meta.resolve('docs/src/error-codes.json')).pathname; |
There was a problem hiding this comment.
One more rebase/merge with my branch should fix the remaining CI issues.
| "19": "Base UI: ComboboxGroupContext is missing. ComboboxGroup parts must be placed within <Combobox.Group>.", | ||
| "20": "Base UI: ComboboxItemContext is missing. ComboboxItem parts must be placed within <Combobox.Item>.", | ||
| "21": "Base UI: <Combobox.Portal> is missing.", | ||
| "22": "Base UI: <Combobox.Popup> and <Combobox.Arrow> must be used within the <Combobox.Positioner> component", |
There was a problem hiding this comment.
This one is missing a closing dot
| "22": "Base UI: <Combobox.Popup> and <Combobox.Arrow> must be used within the <Combobox.Positioner> component", | |
| "22": "Base UI: <Combobox.Popup> and <Combobox.Arrow> must be used within the <Combobox.Positioner> component.", |
I would go as far as throwing when an error minified doesn't have a closing dot 😄
| @@ -0,0 +1,18 @@ | |||
| # Production error | |||
There was a problem hiding this comment.
It feels like we should change this to improve the DX:
- People arriving here would see stuff like this in their console:
- If we create 73 pages with the same H1 and title, that looks like duplication of content; it feels like it defeats the point of slowing down the build time.
So I would expect to see the error code somewhere, like it does in https://react.dev/errors/130.
It's in the h1, it doesn't have to be there, but one option.
There was a problem hiding this comment.
Off-topic. Speaking of the error DX, I wonder about this pattern: having error messages + links to https://nextjs.org/docs/messages/missing-suspense-with-csr-bailout.
But it feels a lot like duplication of content. Meaning this should be in the docs. For example missingSuspenseWithCSRBailout is only documented in their error page 🙈.
So it's probably a no-no, the links in the errors should push back to the docs, that link should be a /r/ link to never get outdated, the docs needs to be clear, and the error message should follow https://www.notion.so/mui-org/Technical-writing-style-guide-7e55b517ac2e489a9ddb6d0f6dd765de#85219822c3194b6f8a49ee08ea82b90a.
Edit: umbrella issue mui/mui-public#222 updated.
| @@ -0,0 +1,36 @@ | |||
| 'use client'; | |||
| import * as React from 'react'; | |||
| import { useSearchParams } from 'next/navigation'; | |||
There was a problem hiding this comment.
Do we need to use the Next.js custom API? What if we were using the window API directly? To what I understand, we will never have a server running to render those pages, so it feels like we are using heavy abstractions (suspense, Next.js router) for no benefits.
There was a problem hiding this comment.
What if we were using the window API directly?
You can't build this hook with current stable browser APIs as you can't listen for history change events. Only way to do it is to monkey patch the history methods, or use the unstable navigation API.
we will never have a server running to render those pages, so it feels like we are using heavy abstractions (suspense, Next.js router) for no benefits.
I don't see what a server has to do with it, it's a next.js page, it will create a router object, whether you use it or not. It's also not a heavy abstraction, what makes you assume that?
There was a problem hiding this comment.
The high-level thoughts (behind my first comment), it feels like we are better off:
- Not using Next.js's specific API when we can use simpler web API for the same outcome. The fewer indirections, the better.
Also, I think we can have general concerns with Next.js' current product management. It feels that they get distracted in rabbit holes where they are not great at, e.g. turbopack: https://youtu.be/w61mLV5nZK0?si=ZksBJLRnUhAVzkYi&t=2283 rather than using RSpack or Rolldown and focus on fixing bugs, stability, and performance issues. And it feels like the more they build deeply integrated Next.js features into Vercel without also owning the portability, the more the framework loses its edge of being a world-class way to deploy a React app (marketing people's perception of it, like how would we feel if AWS were leading PostgreSQL and perceived as building AWS-specific features in PostgreSQL). - Not using Suspense when we can avoid it. It has patterns that need to be learned, and that feel not easy to figure out, e.g., [React 19] Suspense throttling behavior (
FALLBACK_THROTTLE_MS) kicks in too often facebook/react#31819. It's about splitting the rendering in chunks, so it feels mostly about supporting: not declaring data dependency in one place (but lazily, e.g., GraphQL), but do we want this by default? It feels like no. Starting with an explicit declaration of data dependencies for the whole page, rendering everything at once, and only breaking down once it hits limits is better.
I don't see what a server has to do with it
Those are error pages; they do not seem to need SEO, so I assumed that we can either SSR them or not.
Now, reading window.location wouldn't work if the page needs to be rendered on the server, but we can also have hybrid handling, core content is SSR, query are client-side rendered.
you can't listen for history change events.
I assumed that history can't trigger (there are no links), that this could be enough:
React.useEffect(() => {
const searchParams = new URLSearchParams(window.location.search);
}, []);It's also not a heavy abstraction, what makes you assume that?
It's was more about the top ⬆️, me trying to see if we can operate with simpler primitives. In this specific case, yeah, it seems there isn't much we can improve.
There was a problem hiding this comment.
Oh, they have a lot of error codes to render, I imagine we will scale to someone similar:
- https://github.com/vercel/next.js/blob/7383140ada70a6360b8bde4f5124ea77c94f8f46/packages/next/errors.json#L811
- https://github.com/facebook/react/blob/f5e96b974073d3ba80dc844d095c49d5b019afe0/scripts/error-codes/codes.json#L554
If the current approach is slow to generate or to deploy for the CDN, I think going with one SSR page for the errors, and each code as a client side rendering, can be a good solution. If it's fast, SSR will be a better UX (I couldn't see a difference right now with 70 pages, maybe Next.js is smart it builds the page once, render each .html concurrenly, and Netlify handles hundreds of thousands of .html without overhead).
There was a problem hiding this comment.
We could avoid generating potentially more than 70 pages by getting the code as well from query params since at the end, there is going to be a client component. We can just pass the errors json as a prop to the client component and generate the error there on client. We'll only generate one page for all the errors this way.
One other thing. On mui.com, we pass the error string through markdown to html conversion as well for rendering links in errors as <a> tags. We should do something similar here as well, albeit on the server side and pass the transformed errors to the client.
As for avoiding Next.js APIs, I don't see how/why we could/should do that. Our whole docs infra for all our repos is based on Next.js. It anyway won't be easy to switch if/when we decide to. We anyways use them on the actual page component that corresponds to the route, not the imported components. Rest of the components are pretty agnostic of the framework, maybe they use the Link component, but that should be it.
There was a problem hiding this comment.
I assumed that history can't trigger (there are no links), that this could be enough:
React.useEffect(() => { const searchParams = new URLSearchParams(window.location.search); }, []);
Probably want to do
const subscribe = () => () => {};
const getSearchParams = memoize((search) => new URLSearchParams(search))
const getSnapshot = () => getSearchParams(window.location.search)
const getServerSnapshot = () => getSearchParams('');
const useSearchParams = () => React.useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);Just to get that client-side mount working correctly. Avoiding unnecessary renders caused by the effect. But really, from a maintainability point of view, I think we should avoid building components that only work if they're used on a page that has zero incoming links. That's just a completely undocumented constraint and there is nothing in place to prevent someone from using this component on another page 6 months from now. The Next.js abstraction is not that bad, it's not slow and replacing it with another router would be trivial should the day ever come, every router implementation has this hook in one form or another.
Now if only we could use that sweet sweet navigation API, then a few well placed event handlers can transform this in a fully working hook:
const subscribe = (cb) => {
window.navigation.addEventListener('navigation', cb);
return () => {
window.navigation.removeEventListener('navigation', cb);
};
};
const getSearchParams = memoize((search) => new URLSearchParams(search))
const getSnapshot = () => getSearchParams(window.location.search)
const getServerSnapshot = () => getSearchParams('');
const useSearchParams = () => React.useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);a man can dream...
|
|
||
| The full error message: | ||
|
|
||
| <ErrorDisplay msg={props.msg} /> |
There was a problem hiding this comment.
Current https://deploy-preview-1463--base-ui.netlify.app/production-error/1
I doubt this is how this should look. It feels like we should render the error red #ff0000, like people will see the error in their console first. This feels better: https://react.dev/errors/130.
There was a problem hiding this comment.
The design can be iterated on by us as well as the Base UI team. The PR is more about setting up the infrastructure for error codes.
| pnpm extract-error-codes | ||
| ``` | ||
|
|
||
| This will update the `./src/error-codes.json` file with the newly extracted errors. |
There was a problem hiding this comment.
| This will update the `./src/error-codes.json` file with the newly extracted errors. | |
| This updates the `./src/error-codes.json` file with the newly extracted errors. |
|
|
||
| Important: If you just altered the text of an error, you are allowed to update the existing error code with the new text in `./src/error-codes.json`, but only under the following conditions: | ||
|
|
||
| 1. There hasn't been an update to the semantic meaning of the error message. Error codes need to outlive Base UI versions, so the same code must mean the same thing across versions. |
There was a problem hiding this comment.
| 1. There hasn't been an update to the semantic meaning of the error message. Error codes need to outlive Base UI versions, so the same code must mean the same thing across versions. | |
| 1. There hasn't been an update to the semantic meaning of the error message. Error codes need to outlive Base UI versions, so the same code must mean the same thing across versions. |
github-actions
bot
added
the
PR: out-of-date
Port core error minification infrastructure:
<FormattedErrorMessage />inside of the mdx to render the formatted error message