Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/hooks/useManifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export interface NpmPackageVersion {
dependencies: Record<string, string>;
devDependencies: Record<string, string>;
optionalDependencies?: Record<string, string>;
peerDependencies?: Record<string, string>;
}

export type PackageManifest = {
Expand Down
65 changes: 64 additions & 1 deletion src/slugs/deps/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const createMockManifest = (
dependencies: Record<string, string>;
devDependencies: Record<string, string>;
optionalDependencies: Record<string, string>;
peerDependencies: Record<string, string>;
}> = {}
): PackageManifest => ({
name: 'test-package',
Expand All @@ -39,6 +40,7 @@ const createMockManifest = (
dependencies: versionData.dependencies || {},
devDependencies: versionData.devDependencies || {},
optionalDependencies: versionData.optionalDependencies,
peerDependencies: versionData.peerDependencies,
},
},
});
Expand Down Expand Up @@ -87,8 +89,51 @@ describe('Deps Component', () => {
});
});

describe('peerDependencies', () => {
it('should display peerDependencies section', () => {
const manifest = createMockManifest({
peerDependencies: {
'react': '^18.0.0',
},
});

render(<Deps manifest={manifest} version="1.0.0" />);

expect(screen.getByText('PeerDependencies (1)')).toBeInTheDocument();
expect(screen.getByText('react')).toBeInTheDocument();
expect(screen.getByText('^18.0.0')).toBeInTheDocument();
});

it('should display empty peerDependencies section when none exist', () => {
const manifest = createMockManifest({
dependencies: { 'lodash': '^4.17.21' },
});

render(<Deps manifest={manifest} version="1.0.0" />);

expect(screen.getByText('PeerDependencies (0)')).toBeInTheDocument();
});

it('should display multiple peerDependencies', () => {
const manifest = createMockManifest({
peerDependencies: {
'react': '^18.0.0',
'react-dom': '^18.0.0',
'next': '^13.0.0',
},
});

render(<Deps manifest={manifest} version="1.0.0" />);

expect(screen.getByText('PeerDependencies (3)')).toBeInTheDocument();
expect(screen.getByText('react')).toBeInTheDocument();
expect(screen.getByText('react-dom')).toBeInTheDocument();
expect(screen.getByText('next')).toBeInTheDocument();
});
});
Comment on lines +92 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This new test suite for peerDependencies is great for coverage. However, it's almost identical to the existing suite for optionalDependencies. To avoid code duplication in tests, you could use describe.each from vitest to parameterize these tests. This would make the test file more concise and easier to maintain if other dependency types need similar tests in the future.

Here's an example of how you could combine the tests for optionalDependencies and peerDependencies:

describe.each([
  { depType: 'optionalDependencies', title: 'OptionalDependencies', pkgs: { 'fsevents': '^2.3.0', 'chokidar': '^3.5.0', 'esbuild': '^0.19.0' } },
  { depType: 'peerDependencies', title: 'PeerDependencies', pkgs: { 'react': '^18.0.0', 'react-dom': '^18.0.0', 'next': '^13.0.0' } },
])('$title', ({ depType, title, pkgs }) => {
  it(`should display ${depType} section`, () => {
    const firstPkg = Object.keys(pkgs)[0];
    const manifest = createMockManifest({
      [depType]: { [firstPkg]: pkgs[firstPkg] },
    });
    render(<Deps manifest={manifest} version=\"1.0.0\" />);
    expect(screen.getByText(`${title} (1)`)).toBeInTheDocument();
    expect(screen.getByText(firstPkg)).toBeInTheDocument();
    expect(screen.getByText(pkgs[firstPkg])).toBeInTheDocument();
  });

  it(`should display empty ${depType} section when none exist`, () => {
    const manifest = createMockManifest({
      dependencies: { 'lodash': '^4.17.21' },
    });
    render(<Deps manifest={manifest} version=\"1.0.0\" />);
    expect(screen.getByText(`${title} (0)`)).toBeInTheDocument();
  });

  it(`should display multiple ${depType}`, () => {
    const manifest = createMockManifest({
      [depType]: pkgs,
    });
    render(<Deps manifest={manifest} version=\"1.0.0\" />);
    expect(screen.getByText(`${title} (3)`)).toBeInTheDocument();
    for (const pkgName of Object.keys(pkgs)) {
      expect(screen.getByText(pkgName)).toBeInTheDocument();
    }
  });
});

Since this would involve refactoring existing tests, you might consider doing it in a follow-up PR.


describe('all dependency types together', () => {
it('should display all three dependency types', () => {
it('should display all four dependency types', () => {
const manifest = createMockManifest({
dependencies: {
'react': '^18.2.0',
Expand All @@ -102,13 +147,17 @@ describe('Deps Component', () => {
optionalDependencies: {
'fsevents': '^2.3.0',
},
peerDependencies: {
'webpack': '^5.0.0',
},
});

render(<Deps manifest={manifest} version="1.0.0" />);

expect(screen.getByText('Dependencies (2)')).toBeInTheDocument();
expect(screen.getByText('DevDependencies (3)')).toBeInTheDocument();
expect(screen.getByText('OptionalDependencies (1)')).toBeInTheDocument();
expect(screen.getByText('PeerDependencies (1)')).toBeInTheDocument();
});

it('should handle missing version data gracefully', () => {
Expand All @@ -121,6 +170,7 @@ describe('Deps Component', () => {
expect(screen.getByText('Dependencies (0)')).toBeInTheDocument();
expect(screen.getByText('DevDependencies (0)')).toBeInTheDocument();
expect(screen.getByText('OptionalDependencies (0)')).toBeInTheDocument();
expect(screen.getByText('PeerDependencies (0)')).toBeInTheDocument();
});
});

Expand All @@ -137,5 +187,18 @@ describe('Deps Component', () => {
const link = screen.getByRole('link', { name: 'fsevents' });
expect(link).toHaveAttribute('href', '/package/fsevents?version=%5E2.3.0');
});

it('should create correct links for peerDependencies', () => {
const manifest = createMockManifest({
peerDependencies: {
'react': '^18.0.0',
},
});

render(<Deps manifest={manifest} version="1.0.0" />);

const link = screen.getByRole('link', { name: 'react' });
expect(link).toHaveAttribute('href', '/package/react?version=%5E18.0.0');
});
Comment on lines +191 to +202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test is a good addition. Similar to my other comment, this test is very similar to the one for optionalDependencies just above it. You could use it.each to combine them and reduce duplication.

Example:

it.each([
  { depType: 'optionalDependencies', pkg: 'fsevents', version: '^2.3.0' },
  { depType: 'peerDependencies', pkg: 'react', version: '^18.0.0' },
])('should create correct links for $depType', ({ depType, pkg, version }) => {
  const manifest = createMockManifest({
    [depType]: { [pkg]: version },
  });

  render(<Deps manifest={manifest} version=\"1.0.0\" />);

  const link = screen.getByRole('link', { name: pkg });
  expect(link).toHaveAttribute('href', `/package/${pkg}?version=${encodeURIComponent(version)}`);
});

This would make the tests more maintainable.

});
});
25 changes: 20 additions & 5 deletions src/slugs/deps/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@ const columns: TableColumnsType<DepRecord> = [
export default function Deps({ manifest, version }: PageProps) {
const depsInfo = React.useMemo(() => {
const versionData = manifest.versions?.[version!];
if (!versionData) return { dependencies: [], devDependencies: [], optionalDependencies: [] };
if (!versionData) return { dependencies: [], devDependencies: [], optionalDependencies: [], peerDependencies: [] };

const deps = versionData.dependencies || {};
const devDeps = versionData.devDependencies || {};
const optionalDeps = versionData.optionalDependencies || {};
const peerDeps = versionData.peerDependencies || {};

return {
dependencies: Object.entries(deps).map(([pkg, spec]) => ({
Expand All @@ -51,15 +52,19 @@ export default function Deps({ manifest, version }: PageProps) {
package: pkg,
spec,
})),
peerDependencies: Object.entries(peerDeps).map(([pkg, spec]) => ({
package: pkg,
spec,
})),
};
}, [manifest, version]);

const { dependencies, devDependencies, optionalDependencies } = depsInfo;
const { dependencies, devDependencies, optionalDependencies, peerDependencies } = depsInfo;

return (
<SizeContainer maxWidth="90%">
<Row gutter={[8, 8]}>
<Col span={8}>
<Col span={6}>
<Card title={`Dependencies (${dependencies.length})`}>
<Table
dataSource={dependencies}
Expand All @@ -69,7 +74,7 @@ export default function Deps({ manifest, version }: PageProps) {
/>
</Card>
</Col>
<Col span={8}>
<Col span={6}>
<Card title={`DevDependencies (${devDependencies.length})`}>
<Table
dataSource={devDependencies}
Expand All @@ -79,7 +84,7 @@ export default function Deps({ manifest, version }: PageProps) {
/>
</Card>
</Col>
<Col span={8}>
<Col span={6}>
<Card title={`OptionalDependencies (${optionalDependencies.length})`}>
<Table
dataSource={optionalDependencies}
Expand All @@ -89,6 +94,16 @@ export default function Deps({ manifest, version }: PageProps) {
/>
</Card>
</Col>
<Col span={6}>
<Card title={`PeerDependencies (${peerDependencies.length})`}>
<Table
dataSource={peerDependencies}
columns={columns}
rowKey={'package'}
pagination={{ size: 'small', pageSize: 50 }}
/>
</Card>
</Col>
</Row>
</SizeContainer>
);
Expand Down