fix(core): clamp Calendar numberOfMonths to 1|2 at runtime#3248
fix(core): clamp Calendar numberOfMonths to 1|2 at runtime#3248durvesh1992 wants to merge 2 commits into
Conversation
|
Someone is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks, console warn makes sense although I don't think process exists in runtime.
| // Clamp to the supported union: anything that isn't exactly `2` falls to `1`. | ||
| const numberOfMonths = numberOfMonthsProp === 2 ? 2 : 1; | ||
| if ( | ||
| process.env.NODE_ENV !== 'production' && |
There was a problem hiding this comment.
This might not always exist and is likely causing the build errors.
process is not guaranteed to exist in the runtime bundle; match the codebase convention (Field/Popover) of an unconditional console.warn.
|
Good catch — |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks for working on this. I was reading through #2704 and thinking about the kinds of guardrails we want for this. Since the type is already constrained by the type I think having additional console warnings is likely not going to be quite as helpful as the type constraint. Given that we don't have a good way to handle in-component errors in a traceable way for consumers (ie if an end user encounters this issue it needs to be logged somewhere for the developer) and we have type safety, I'm thinking we can skip the console warnings for now. The defensive clamping seems okay although I'm not fully sure we need it yet. The main fix would be to avoid getting into this situation in the docsite's properties panel by having it read the 1 | 2 union type and render a selector instead.
Summary
numberOfMonthsonCalendar(and theDateInput/DateTimeInputthat forward to it) is typed1 | 2, but nothing enforced that at runtime. A free-form caller — or the docsite Properties control — could pass0(renders no months) or1000(locks the page up trying to render 1000 month grids inArray.from({length: numberOfMonths})).This clamps the value to the supported union in
Calendar: anything that isn't exactly2falls back to1, with a dev-onlyconsole.warnwhen an out-of-range value is passed.DateInputandDateTimeInputinherit the guard for free since they forward the prop straight to<Calendar>.Note: the docsite should also render a 1/2 toggle instead of a free-form number input for small-literal-union props (item 3 in the issue) — that's a separate docsite-side change and not included here.
Test plan
numberOfMonths={1000}and={0}both render exactly one month and emit the dev warning.={2}) still pass — 32/32 Calendar tests green.Closes #2704