fix(peers): stop the barrel from requiring optional peer dependencies#1737
Conversation
`@mui/x-date-pickers` and `date-fns` are declared optional under
`peerDependenciesMeta`, so a consumer is entitled to skip them. But
`src/index.tsx` re-exports nearly everything, which means a module-scope
import of an optional peer in ANY barrel-reachable file breaks that promise
for everyone: `import { Button } from '@sistent/sistent'` throws
`Cannot find module 'date-fns'` before a single component renders, and the
message names sistent rather than the peer the consumer deliberately did not
install.
#1732 fixed one instance of this class by deferring `@mui/x-date-pickers` to
`React.lazy`. `UniversalFilter` was still importing `subDays`/`subMonths`/
`subYears` from `date-fns` at module scope for its quick-range presets, so
the barrel remained unusable on a clean install (#1735). Meshery UI happens
to have `date-fns` installed and never saw it; consumers that do not, do.
The presets now use `src/utils/date.utils.ts`, a new dependency-free leaf
module. It is deliberately NOT folded into `time.utils.tsx`, which drags in
`moment` and a React component - pure date arithmetic should be reachable
without a rendering stack behind it. Semantics match `date-fns` exactly,
including end-of-month clamping (`subtractMonths(May 31, 3)` is Feb 28, not
Mar 3) and local-time day arithmetic so a range spanning a DST boundary does
not drift by an hour.
`optionalPeerDependencies.test.ts` guards the whole bug class rather than
this one component: it walks the source tree and fails on any eager import
of an optional peer, listing the offending paths so the failure message is
also the remediation. It also asserts the peers are still marked optional,
so if that premise ever changes the assertions get revisited rather than
silently kept.
Also carried in this change:
- `DateTimePicker` now wraps its lazy peer import so a missing peer surfaces
as a message naming the peer, instead of a raw module-not-found thrown
mid-render from inside a promise. Deferring the resolution deferred the
failure too; an unactionable async crash is no improvement on an
unactionable sync one.
- `time.utils.tsx` imports `CustomTooltip` from its leaf path instead of the
`../custom` barrel, breaking a `custom/X -> utils -> custom/index -> X`
cycle and no longer forcing consumers of a date helper to load the entire
component library.
- `permissions.tsx` types `createCanShow`'s bus as a structural
`ReasonEventPublisher`. `EventBus<T>` is invariant in `T`, so the host's
`EventBus<MesheryExtensionEvent>` - the bus the contract tells every host
to declare - was not assignable to `EventBus<ReasonEvent>` and the
integration failed to compile. It also publishes via the named
`MESHERY_EXTENSION_EVENT.MissingPermission` handle so a contract rename
breaks this publish site rather than silently ceasing to match the
subscriber, and drops a stray `console.log` from the click path.
- The barrel exports `HasKeyProps`, `InvertAction`, `ReasonEvent` and
`ReasonEventPublisher` alongside `createCanShow`; without them a consumer
cannot name the types of the wrapper it builds and falls back to `any`.
Fixes #1735
Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds dependency-free date arithmetic, removes eager ChangesOptional dependency and date handling
Permission event contract and exports
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__testing__/optionalPeerDependencies.test.ts`:
- Around line 20-28: The eagerImportPattern must also match side-effect imports
such as import 'date-fns' and re-exports such as export { x } from 'date-fns'.
Update the pattern’s import syntax alternatives while preserving existing
require(), named/default import, specifier escaping, subpath matching, and
exclusion of await import().
In `@src/base/DateTimePicker/DateTimePicker.tsx`:
- Around line 32-36: Update the remediation message in the DateTimePicker error
construction to reference the UniversalFilter datePicker prop instead of
dateRange, while preserving the existing optional-dependency guidance and error
cause.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40a326ba-529c-4d7a-9621-d858b96878e1
📒 Files selected for processing (9)
src/__testing__/date.utils.test.tssrc/__testing__/optionalPeerDependencies.test.tssrc/base/DateTimePicker/DateTimePicker.tsxsrc/custom/UniversalFilter.tsxsrc/custom/permissions.tsxsrc/index.tsxsrc/utils/date.utils.tssrc/utils/index.tssrc/utils/time.utils.tsx
The guard only matched `import ... from 'x'` and `require('x')`, so three
forms that resolve the module just as eagerly slipped past it:
- `export ... from 'x'` - and this is the one that matters, because
`src/index.tsx` is built almost entirely out of `export ... from`. The
barrel is precisely where this bug class originates, and the guard was
blind to the barrel's own syntax.
- `import 'x'` - a side-effect import binds nothing, which is exactly why it
is easy to miss by eye and why a scanner should catch it.
- the subpath variants of both.
A guard that silently stops matching reports the same clean result as a clean
codebase, so the detector is now pinned by its own cases: thirteen assertions
covering each form it must flag and each it must not - dynamic `import()`,
type-only imports and re-exports, unrelated specifiers, and prose mentions.
Verified by mutation: adding `export { subDays } from 'date-fns'` to
`src/index.tsx` now fails the suite and names `index.tsx`, where before it
passed.
`import type` stripping was widened to `export type` to match. Without it the
new `export ... from` branch would fail a correct type re-export, and the only
way to satisfy the guard would be to delete a type.
Also corrects the DateTimePicker remediation message, which named a
`<UniversalFilter dateRange>` prop that does not exist - the prop is
`datePicker`. An error message whose purpose is to be actionable has to name
the real API.
Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
The guard checked that the two peers it scans are still marked optional, but not the converse: marking a *third* peer optional created a peer nothing scanned, and the suite kept passing while the bug class quietly reopened behind it. The list of scanned peers and the optional peers declared in package.json now have to agree exactly. `react` and `react-dom` are also marked optional and are named in an explicit exemption list rather than skipped silently - every component imports React at module scope and always will, so scanning for it would fail on the whole tree. Recording them as a decision keeps the completeness check meaningful instead of making it something to loosen the first time it fires. Verified by mutation: marking `@rjsf/core` optional now fails the suite and names it. Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Two corrections to the optional-peer section. 'CI cannot see this' went stale when #1737 landed src/__testing__/optionalPeerDependencies.test.ts, which scans src/ for load-time imports of an enforced optional peer and fails jest. Describe the two guards as complementary instead: the test covers source, the packed-consumer check covers what the bundler emitted and what is actually on npm - neither subsumes the other. Point at the test as the source of truth for which peers are enforced, since it is where the react / react-dom exemption is recorded. The absence check only looked for date-fns and only warned, so it could print a warning and then report a passing load with @mui/x-date-pickers still installed - a false pass in exactly the situation the check exists to catch. It now walks the enforced peers and exits nonzero. Deliberately not enumerating every peer under peerDependenciesMeta: react and react-dom are marked optional there but are imported at module scope by every component, so requiring their absence could never pass. That exemption is documented in the test rather than invented here. Verified verbatim against the published 0.21.40 tarball: clean consumer loads (exit 0); with date-fns force-installed it aborts (exit 1). Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
Fixes #1735. Completes the optional-peer work started in #1732.
The bug class
@mui/x-date-pickersanddate-fnsare both declared optional underpeerDependenciesMeta, so a consumer is entitled to skip them. Butsrc/index.tsxre-exports nearly everything, which means a module-scope import of an optional peer in any barrel-reachable file breaks that promise for everyone - not just for people who render the component that needs it:The message names sistent rather than the peer the consumer deliberately did not install, which is what makes it expensive to diagnose downstream.
#1732 fixed one instance by deferring
@mui/x-date-pickerstoReact.lazy. One instance remained:UniversalFilterstill importedsubDays/subMonths/subYearsfromdate-fnsat module scope for its quick-range presets, so the barrel was still unusable on a clean install. Meshery UI happens to havedate-fnsinstalled and never saw it; Kanvas does not, and is blocked on this (layer5labs/meshery-extensions#4303).The fix
New
src/utils/date.utils.ts-subtractDays/subtractMonths/subtractYears, dependency-free.Deliberately a leaf module with no imports at all, and deliberately not folded into
time.utils.tsx, which pulls inmomentand a React component: pure date arithmetic should be reachable without dragging a rendering stack behind it.momentwas considered and rejected for the same reason - swapping one heavyweight import for another does not fix an import-graph problem.Semantics match
date-fnsexactly:subtractMonths(May 31, 3)is Feb 28, not Mar 3 (plainsetMonthoverflows)subtractYears(Feb 29 2024, 1)is Feb 28 2023, not Mar 1n * 86400000ms, so a range spanning a DST boundary keeps its wall-clock time instead of drifting an hourPreventing recurrence
src/__testing__/optionalPeerDependencies.test.tsguards the whole class, not this one component. It walks the source tree and fails on any eager import of an optional peer, listing the offending paths - so the failure message is also the remediation. It additionally asserts the peers are still marked optional, so if that premise ever changes the assertions get revisited rather than silently kept.This matters because, as noted in #1736, CI cannot otherwise catch this: the repo always has its own devDependencies installed, so the breakage only appears in a downstream clean install. This test makes it visible in-repo.
src/__testing__/date.utils.test.tscovers the helpers themselves, including the clamping and DST cases above.Also carried in this change
DateTimePickerwraps its lazy peer import so a missing peer surfaces as a message naming the peer, rather than a raw module-not-found thrown mid-render from inside a promise. Deferring the resolution deferred the failure too, and an unactionable async crash is no improvement on an unactionable sync one.time.utils.tsximportsCustomTooltipfrom its leaf path instead of the../custombarrel. That closed acustom/X -> utils -> custom/index -> Xcycle and forced every consumer of a date helper to load the entire component library (including, viaMarkdown, ESM-onlyreact-markdown).permissions.tsxtypescreateCanShow's bus as a structuralReasonEventPublisher.EventBus<T>is invariant inT- it both acceptsTinpublishand yieldsTfromon- so the host'sEventBus<MesheryExtensionEvent>, the bus the contract tells every host to declare, was not assignable toEventBus<ReasonEvent>and the integration failed to compile. It also publishes via the namedMESHERY_EXTENSION_EVENT.MissingPermissionhandle so a contract rename breaks this publish site rather than silently ceasing to match the subscriber across the bundle boundary, and drops a strayconsole.logfrom the click path.HasKeyProps,InvertAction,ReasonEventandReasonEventPublisheralongsidecreateCanShow; without them a consumer cannot name the types of the wrapper it builds around the returned component and falls straight back toany.Verification
npm test- 24 suites, 443 tests passing, including the 32 new onesnpm run lintclean;prettier --checkclean on every touched file (the ~82 pre-existing failures elsewhere are unrelated, see docs: add AGENTS.md with release, optional-peer, and repo-state notes #1736)npm run buildcleandate-fnsno longer appears as a module specifier anywhere indist/index.jsordist/index.mjs. The three@mui/x-date-pickers/*specifiers that remain are insideimport(...)calls only - no static import survives.subtractDays/subtractMonths/subtractYearsagainstdate-fns@4.4.0- every day from 2019-2027 at three times of day across a range of amounts - produced 0 mismatches. The "semantics match exactly" claim is measured, not asserted.node_modules/date-fnsandnode_modules/@mui/x-date-pickersboth removed,require('./dist/index.js')succeeds and resolves 719 exports. That is the exact scenario that throws onmastertoday.Guard hardening (from review)
The first version of the guard only matched
import ... from 'x'andrequire('x'). Review caught that it missed several forms that resolve the module just as eagerly, and one of them mattered a great deal:export ... from 'x'-src/index.tsxis built almost entirely out ofexport ... from. The barrel is precisely where this bug class originates, and the guard was blind to the barrel's own syntax. Addingexport { subDays } from 'date-fns'tosrc/index.tsxpassed the original guard; it now fails and namesindex.tsx.import 'x'- a side-effect import binds nothing, which is exactly why it is easy to miss by eye.The detector is now pinned by its own cases (13 assertions for what it must and must not flag: dynamic
import(), type-only imports and re-exports, unrelated specifiers, prose mentions). A scanner that quietly stops matching a form reports the same clean result as a clean codebase, so the pattern needed to stop being the untested part of the test.import typestripping was widened toexport typeto match - otherwise the newexport ... frombranch would fail a correct type re-export, and the only way to satisfy the guard would be to delete a type.Separately,
reactandreact-domare also marked optional inpackage.jsonbut are not scanned (every component imports React at module scope and always will). They are now named in an explicit exemption list, and a new assertion requires the scanned set plus the exemptions to equal the declared optional peers exactly - so marking a new peer optional fails the suite rather than silently creating a peer nothing guards. Verified by mutation with@rjsf/core.Also fixed: the
DateTimePickerremediation message named a<UniversalFilter dateRange>prop that does not exist (it isdatePicker). An error message whose entire purpose is to be actionable has to name the real API.Out of scope, but noticed
dist/index.mjsemitsimport ... from "lodash/debounce"(fromsrc/custom/SearchBar.tsx), which Node's ESM resolver rejects - it needs the.jsextension. Bundlers paper over it, so it does not affect Meshery UI or Kanvas, and it is pre-existing onmasterand unrelated to optional peers. Filed separately rather than folded in here.Note for reviewers
masterpicked up two contract commits (5eeff36, 61619b2) after the base this work was written against, which refinedmesheryExtensionContract.tsand its tests along a different line. Those landed versions are kept as-is here - this PR does not touch either file, so nothing already onmasteris reverted.#1736 lists #1735 as a "known live instance" of the barrel trap; that line goes stale once this merges and is worth a one-line update there.
Summary by CodeRabbit
subtractDays,subtractMonths,subtractYears) that return newDatevalues, preserve time-of-day, and clamp to valid month-end/leap-year dates.