-
Notifications
You must be signed in to change notification settings - Fork 227
fix(peers): stop the barrel from requiring optional peer dependencies #1737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
99934cb
fix(peers): stop the barrel from requiring optional peer dependencies
CodeAhmedJamil 45e75e3
fix(peers): catch re-exports and side-effect imports in the peer guard
CodeAhmedJamil 09ee260
test(peers): fail when a new peer is marked optional but never scanned
CodeAhmedJamil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { subtractDays, subtractMonths, subtractYears } from '../utils/date.utils'; | ||
|
|
||
| // These replaced `date-fns` (an optional peer dependency that the package | ||
| // barrel was importing eagerly). The end-of-month clamping is the part that is | ||
| // easy to get wrong and silently ships a filter range off by three days. | ||
|
|
||
| describe('subtractDays', () => { | ||
| it('walks back across a month boundary', () => { | ||
| expect(subtractDays(new Date(2025, 2, 5), 7)).toEqual(new Date(2025, 1, 26)); | ||
| }); | ||
|
|
||
| it('walks back across a year boundary', () => { | ||
| expect(subtractDays(new Date(2025, 0, 3), 7)).toEqual(new Date(2024, 11, 27)); | ||
| }); | ||
|
|
||
| it('preserves the time of day', () => { | ||
| expect(subtractDays(new Date(2025, 5, 15, 13, 45, 30, 250), 30)).toEqual( | ||
| new Date(2025, 4, 16, 13, 45, 30, 250) | ||
| ); | ||
| }); | ||
|
|
||
| it('does not mutate its argument', () => { | ||
| const original = new Date(2025, 5, 15); | ||
| subtractDays(original, 30); | ||
| expect(original).toEqual(new Date(2025, 5, 15)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('subtractMonths', () => { | ||
| it('subtracts whole months when the day exists in the target month', () => { | ||
| expect(subtractMonths(new Date(2025, 7, 15), 3)).toEqual(new Date(2025, 4, 15)); | ||
| }); | ||
|
|
||
| it('clamps to the last day of a shorter target month instead of rolling over', () => { | ||
| // The naive `setMonth(getMonth() - 3)` on May 31st yields March 3rd, which | ||
| // would silently widen a "last 3 months" range by three days. | ||
| expect(subtractMonths(new Date(2025, 4, 31), 3)).toEqual(new Date(2025, 1, 28)); | ||
| expect(subtractMonths(new Date(2025, 9, 31), 1)).toEqual(new Date(2025, 8, 30)); | ||
| }); | ||
|
|
||
| it('clamps into a leap February', () => { | ||
| expect(subtractMonths(new Date(2024, 2, 31), 1)).toEqual(new Date(2024, 1, 29)); | ||
| }); | ||
|
|
||
| it('crosses a year boundary', () => { | ||
| expect(subtractMonths(new Date(2025, 1, 10), 6)).toEqual(new Date(2024, 7, 10)); | ||
| }); | ||
|
|
||
| it('preserves the time of day while clamping', () => { | ||
| expect(subtractMonths(new Date(2025, 4, 31, 9, 30), 3)).toEqual(new Date(2025, 1, 28, 9, 30)); | ||
| }); | ||
|
|
||
| it('does not mutate its argument', () => { | ||
| const original = new Date(2025, 4, 31); | ||
| subtractMonths(original, 3); | ||
| expect(original).toEqual(new Date(2025, 4, 31)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('subtractYears', () => { | ||
| it('subtracts whole years', () => { | ||
| expect(subtractYears(new Date(2025, 6, 4), 1)).toEqual(new Date(2024, 6, 4)); | ||
| }); | ||
|
|
||
| it('clamps Feb 29th to Feb 28th on a non-leap target year', () => { | ||
| expect(subtractYears(new Date(2024, 1, 29), 1)).toEqual(new Date(2023, 1, 28)); | ||
| }); | ||
|
|
||
| it('keeps Feb 29th when the target year is also a leap year', () => { | ||
| expect(subtractYears(new Date(2024, 1, 29), 4)).toEqual(new Date(2020, 1, 29)); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| /** | ||
| * `@mui/x-date-pickers` and `date-fns` are declared OPTIONAL peer dependencies, | ||
| * so a consumer is entitled to skip them. Any module-scope import of either one | ||
| * that is reachable from the package barrel breaks that promise: `import | ||
| * { Button } from '@sistent/sistent'` then throws `Cannot find module` before a | ||
| * single component renders, and the message points at sistent rather than at the | ||
| * peer the consumer chose not to install. | ||
| * | ||
| * This is the regression guard for that class of bug — it is the import graph, | ||
| * not any one component, that has to stay clean. `DateTimePicker` was fixed by | ||
| * deferring to `React.lazy`; `UniversalFilter` was still pulling `date-fns` in | ||
| * eagerly for its quick-range presets. | ||
| */ | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
|
|
||
| const SRC = path.resolve(__dirname, '..'); | ||
| const OPTIONAL_PEERS = ['@mui/x-date-pickers', 'date-fns']; | ||
|
|
||
| /** | ||
| * Peers marked optional that this scan deliberately does not enforce. | ||
| * | ||
| * `react` / `react-dom` are marked optional in `package.json`, but every | ||
| * component in this library imports React at module scope and always will - | ||
| * scanning for it would fail on the entire `src` tree on the first run. They | ||
| * are listed here rather than silently skipped so the exemption is a decision | ||
| * on the record instead of an omission, and so the completeness check below | ||
| * still notices if a *new* peer is ever marked optional. | ||
| */ | ||
| const UNENFORCED_OPTIONAL_PEERS = ['react', 'react-dom']; | ||
|
|
||
| /** | ||
| * Every form that makes the module resolve at load time, and only those. | ||
| * | ||
| * - `import ... from 'x'` and `require('x')` - the obvious ones. | ||
| * - `import 'x'` - a side-effect import still resolves the module; it binds | ||
| * nothing, which is exactly why it is easy to miss by eye. | ||
| * - `export ... from 'x'` - a re-export resolves it too, and this is the form | ||
| * most likely to reintroduce the bug: `src/index.tsx` is built almost | ||
| * entirely out of `export ... from`, and it is the barrel that turns one | ||
| * module's import into every consumer's problem. | ||
| * | ||
| * `await import('x')` is deliberately NOT matched - deferring the resolution is | ||
| * the fix, not the defect. | ||
| */ | ||
| const eagerImportPattern = (specifier: string) => { | ||
| const escaped = specifier.replace(/[/\\^$*+?.()|[\]{}]/g, '\\$&'); | ||
| // A subpath (`x/sub`) resolves the same package, so it counts. | ||
| const target = String.raw`['"]${escaped}(?:/[^'"]*)?['"]`; | ||
|
|
||
| return new RegExp( | ||
| String.raw`(?:^|[^.\w])(?:` + | ||
| // `import ... from 'x'` / `export ... from 'x'` | ||
| String.raw`(?:import|export)\s[^;]*?from\s*${target}` + | ||
| // `import 'x'` - side-effect only. The absence of `(` is what keeps this | ||
| // from also matching the dynamic `import('x')` above it. | ||
| String.raw`|import\s*${target}` + | ||
| // `require('x')` | ||
| String.raw`|require\s*\(\s*${target}` + | ||
| String.raw`)`, | ||
| 'm' | ||
| ); | ||
| }; | ||
|
|
||
| const collectSourceFiles = (dir: string): string[] => | ||
| fs.readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { | ||
| const full = path.join(dir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| return entry.name === '__testing__' ? [] : collectSourceFiles(full); | ||
| } | ||
| return /\.tsx?$/.test(entry.name) && !/\.test\.tsx?$/.test(entry.name) ? [full] : []; | ||
| }); | ||
|
|
||
| /** | ||
| * Type-only imports and re-exports are erased by the compiler and never reach | ||
| * the bundle, so they are not what breaks a consumer without the peer | ||
| * installed - `DateTimePicker` legitimately names its props type this way. | ||
| * | ||
| * `export type` is stripped as well as `import type`: the pattern above matches | ||
| * `export ... from`, so without this a perfectly correct type re-export would | ||
| * fail the guard and the only way to satisfy it would be to delete a type. | ||
| */ | ||
| const stripTypeOnlyStatements = (source: string): string => | ||
| source.replace(/^\s*(?:import|export)\s+type\s[^;]*;/gm, ''); | ||
|
|
||
| describe('optional peer dependencies stay out of the eager import graph', () => { | ||
| const sourceFiles = collectSourceFiles(SRC); | ||
|
|
||
| it('finds source files to check', () => { | ||
| expect(sourceFiles.length).toBeGreaterThan(100); | ||
| }); | ||
|
|
||
| it.each(OPTIONAL_PEERS)('no module eagerly imports %s', (peer) => { | ||
| const pattern = eagerImportPattern(peer); | ||
|
|
||
| const offenders = sourceFiles.filter((file) => | ||
| pattern.test(stripTypeOnlyStatements(fs.readFileSync(file, 'utf8'))) | ||
| ); | ||
|
|
||
| // Listing the paths rather than asserting a count: on failure the message | ||
| // names the exact modules to defer, which is the whole remediation. | ||
| expect(offenders.map((file) => path.relative(SRC, file))).toEqual([]); | ||
| }); | ||
|
|
||
| // The scan above is only ever as good as the pattern behind it, and a pattern | ||
| // that quietly stops matching a form reports the same clean result as a | ||
| // codebase that is actually clean. These cases pin the forms it must catch - | ||
| // `export ... from` and the side-effect `import 'x'` were both missed by the | ||
| // first version of this guard, which would have let the barrel reintroduce | ||
| // the exact bug this file exists to prevent. | ||
| describe('the detector itself', () => { | ||
| const flags = (source: string) => | ||
| eagerImportPattern('date-fns').test(stripTypeOnlyStatements(source)); | ||
|
|
||
| it.each([ | ||
| ["import { subDays } from 'date-fns';", 'named import'], | ||
| ["import subDays from 'date-fns';", 'default import'], | ||
| ["import 'date-fns';", 'side-effect import, binds nothing'], | ||
| ["import 'date-fns/locale/en-US';", 'side-effect subpath import'], | ||
| ["export { subDays } from 'date-fns';", 're-export'], | ||
| ["export * from 'date-fns';", 'star re-export'], | ||
| ["const { subDays } = require('date-fns');", 'require'], | ||
| ["import { formatDistance } from 'date-fns/formatDistance';", 'subpath import'] | ||
| ])('flags %j (%s)', (source) => { | ||
| expect(flags(source)).toBe(true); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [ | ||
| "const { subDays } = await import('date-fns');", | ||
| 'dynamic import is the fix, not the defect' | ||
| ], | ||
| ["import type { Duration } from 'date-fns';", 'type-only import is erased'], | ||
| ["export type { Duration } from 'date-fns';", 'type-only re-export is erased'], | ||
| ["import { subtractDays } from '../utils/date.utils';", 'unrelated specifier'], | ||
| ['// mentions date-fns in a comment', 'prose is not an import'] | ||
| ])('does not flag %j (%s)', (source) => { | ||
| expect(flags(source)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('package.json stays in step with what is scanned', () => { | ||
| const pkg = JSON.parse(fs.readFileSync(path.resolve(SRC, '..', 'package.json'), 'utf8')) as { | ||
| peerDependenciesMeta?: Record<string, { optional?: boolean }>; | ||
| }; | ||
| const declaredOptional = Object.entries(pkg.peerDependenciesMeta ?? {}) | ||
| .filter(([, meta]) => meta?.optional) | ||
| .map(([name]) => name); | ||
|
|
||
| it.each(OPTIONAL_PEERS)('keeps %s marked optional', (peer) => { | ||
| // If a peer stops being optional this test's premise changes and the | ||
| // eager-import assertions above should be revisited, not silently kept. | ||
| expect(declaredOptional).toContain(peer); | ||
| }); | ||
|
|
||
| // Without this, marking a new peer optional would quietly create a peer | ||
| // nothing scans - the guard would keep passing while the class of bug it | ||
| // exists to prevent reopened behind it. | ||
| it('scans (or explicitly exempts) every peer marked optional', () => { | ||
| expect(declaredOptional.slice().sort()).toEqual( | ||
| [...OPTIONAL_PEERS, ...UNENFORCED_OPTIONAL_PEERS].sort() | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.