Skip to content

Commit

Permalink
Merge pull request storybookjs#21114 from storybookjs/20663-coalesce-…
Browse files Browse the repository at this point in the history 
…errors

Core: Coalesce multiple indexing errors into one
  • Loading branch information
@tmeasday
tmeasday authored Feb 20, 2023
2 parents da186a0 + c22c537 commit 35ece38
Show file tree
Hide file tree
Showing 8 changed files with 204 additions and 106 deletions.
1 change: 0 additions & 1 deletion ...stories/stories-mdx/csf-in-mdx.stories.js → ...ies/stories-mdx/csf-in-mdx.non-stories.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable storybook/default-exports */
// NOTE: commented out default since these stories are re-exported
// in the primary file './csf-docs-with-mdx-docs.stories.mdx'
//
Expand Down
2 changes: 1 addition & 1 deletion code/addons/docs/template/stories/stories-mdx/csf-in-mdx.stories.mdx
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Meta, Story, Canvas, ArgsTable } from '@storybook/addon-docs';
import { global as globalThis } from '@storybook/global';
import * as Csf from './csf-in-mdx.stories.js';
import * as Csf from './csf-in-mdx.non-stories.js';

<Meta component={globalThis.Components.Button} />

Expand Down
51 changes: 51 additions & 0 deletions code/lib/core-server/src/utils/IndexingError.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import slash from 'slash';

export class IndexingError extends Error {
importPaths: string[];

constructor(message: string, importPaths: string[], stack?: string) {
super();
this.message = message;
this.importPaths = importPaths;
if (stack) {
this.stack = stack;
}
}

pathsString() {
if (this.importPaths.length === 1) {
return `${slash(this.importPaths[0])}`;
}

return `${this.importPaths.map(slash).join(',')}`;
}

toString() {
return `${this.pathsString()}: ${this.message}`;
}
}

export class MultipleIndexingError extends Error {
constructor(public indexingErrors: IndexingError[]) {
super();

if (this.indexingErrors.length === 0) throw new Error('Unexpected empty error list');

if (this.indexingErrors.length === 1) {
const [err] = this.indexingErrors;
this.message = `Unable to index ${err.pathsString()}`;
} else {
this.message = `Unable to index files:\n${this.indexingErrors
.map((err) => `- ${err}`)
.join('\n')}`;
}
}

toString() {
if (this.indexingErrors.length === 1) {
return `${this.message}:\n ${this.indexingErrors[0].stack}`;
}

return this.message;
}
}
21 changes: 8 additions & 13 deletions code/lib/core-server/src/utils/StoryIndexGenerator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { loadCsf, getStorySortParameter } from '@storybook/csf-tools';
import { toId } from '@storybook/csf';
import { logger } from '@storybook/node-logger';

import { StoryIndexGenerator, DuplicateEntriesError } from './StoryIndexGenerator';
import { StoryIndexGenerator } from './StoryIndexGenerator';

jest.mock('@storybook/csf-tools');
jest.mock('@storybook/csf', () => {
Expand Down Expand Up @@ -446,8 +446,8 @@ describe('StoryIndexGenerator', () => {
const generator = new StoryIndexGenerator([csfSpecifier, docsSpecifier], autodocsOptions);
await generator.initialize();

await expect(generator.getIndex()).rejects.toThrowError(
`You created a component docs page for B (./errors/MetaOfClashingDefaultName.mdx), but also tagged the CSF file (./src/B.stories.ts) with 'autodocs'. This is probably a mistake.`
await expect(generator.getIndex()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Unable to index ./errors/MetaOfClashingDefaultName.mdx,./src/B.stories.ts"`
);
});

Expand All @@ -465,8 +465,8 @@ describe('StoryIndexGenerator', () => {
const generator = new StoryIndexGenerator([csfSpecifier, docsSpecifier], autodocsOptions);
await generator.initialize();

await expect(generator.getIndex()).rejects.toThrowError(
`You created a component docs page for B (./errors/B.mdx), but also tagged the CSF file (./src/B.stories.ts) with 'autodocs'. This is probably a mistake.`
await expect(generator.getIndex()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Unable to index ./errors/B.mdx,./src/B.stories.ts"`
);
});

Expand Down Expand Up @@ -896,8 +896,8 @@ describe('StoryIndexGenerator', () => {
options
);
await generator.initialize();
await expect(() => generator.getIndex()).rejects.toThrowError(
/Could not find "..\/A.stories" for docs file/
await expect(() => generator.getIndex()).rejects.toThrowErrorMatchingInlineSnapshot(
`"Unable to index ./src/docs2/MetaOf.mdx"`
);
});
});
Expand Down Expand Up @@ -1004,12 +1004,7 @@ describe('StoryIndexGenerator', () => {
};
expect(() => {
generator.chooseDuplicate(mockEntry, mockEntry);
}).toThrow(
new DuplicateEntriesError(`Duplicate stories with id: ${mockEntry.id}`, [
mockEntry,
mockEntry,
])
);
}).toThrowErrorMatchingInlineSnapshot(`"Duplicate stories with id: StoryId"`);
});
});
});
Expand Down
168 changes: 82 additions & 86 deletions code/lib/core-server/src/utils/StoryIndexGenerator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ import type {
import { userOrAutoTitleFromSpecifier, sortStoriesV7 } from '@storybook/preview-api';
import { normalizeStoryPath } from '@storybook/core-common';
import { logger } from '@storybook/node-logger';
import { getStorySortParameter, NoMetaError } from '@storybook/csf-tools';
import { getStorySortParameter } from '@storybook/csf-tools';
import { toId } from '@storybook/csf';
import { analyze } from '@storybook/docs-mdx';
import { autoName } from './autoName';
import { IndexingError, MultipleIndexingError } from './IndexingError';

/** A .mdx file will produce a docs entry */
type DocsCacheEntry = DocsIndexEntry;
Expand All @@ -37,7 +38,7 @@ type StoriesCacheEntry = {
};
type ErrorEntry = {
type: 'error';
err: Error;
err: IndexingError;
};
type CacheEntry = false | StoriesCacheEntry | DocsCacheEntry | ErrorEntry;
type SpecifierStoriesCache = Record<Path, CacheEntry>;
Expand All @@ -51,16 +52,6 @@ export function isMdxEntry({ tags }: DocsIndexEntry) {
return !tags?.includes(AUTODOCS_TAG) && !tags?.includes(STORIES_MDX_TAG);
}

export class DuplicateEntriesError extends Error {
entries: IndexEntry[];

constructor(message: string, entries: IndexEntry[]) {
super();
this.message = message;
this.entries = entries;
}
}

const makeAbsolute = (otherImport: Path, normalizedPath: Path, workingDir: Path) =>
otherImport.startsWith('.')
? slash(
Expand Down Expand Up @@ -176,7 +167,15 @@ export class StoryIndexGenerator {
try {
entry[absolutePath] = await updater(specifier, absolutePath, entry[absolutePath]);
} catch (err) {
entry[absolutePath] = { type: 'error', err };
const relativePath = `.${path.sep}${path.relative(
this.options.workingDir,
absolutePath
)}`;

entry[absolutePath] = {
type: 'error',
err: new IndexingError(err.message, [relativePath], err.stack),
};
}
})
);
Expand Down Expand Up @@ -243,61 +242,53 @@ export class StoryIndexGenerator {
async extractStories(specifier: NormalizedStoriesSpecifier, absolutePath: Path) {
const relativePath = path.relative(this.options.workingDir, absolutePath);
const entries = [] as IndexEntry[];
try {
const importPath = slash(normalizeStoryPath(relativePath));
const makeTitle = (userTitle?: string) => {
return userOrAutoTitleFromSpecifier(importPath, specifier, userTitle);
};

const storyIndexer = this.options.storyIndexers.find((indexer) =>
indexer.test.exec(absolutePath)
);
if (!storyIndexer) {
throw new Error(`No matching story indexer found for ${absolutePath}`);
}
const csf = await storyIndexer.indexer(absolutePath, { makeTitle });
const importPath = slash(normalizeStoryPath(relativePath));
const makeTitle = (userTitle?: string) => {
return userOrAutoTitleFromSpecifier(importPath, specifier, userTitle);
};

const componentTags = csf.meta.tags || [];
csf.stories.forEach(({ id, name, tags: storyTags, parameters }) => {
if (!parameters?.docsOnly) {
const tags = [...(storyTags || componentTags), 'story'];
entries.push({ id, title: csf.meta.title, name, importPath, tags, type: 'story' });
}
});
const storyIndexer = this.options.storyIndexers.find((indexer) =>
indexer.test.exec(absolutePath)
);
if (!storyIndexer) {
throw new Error(`No matching story indexer found for ${absolutePath}`);
}
const csf = await storyIndexer.indexer(absolutePath, { makeTitle });

if (csf.stories.length) {
const { autodocs } = this.options.docs;
const componentAutodocs = componentTags.includes(AUTODOCS_TAG);
const autodocsOptedIn = autodocs === true || (autodocs === 'tag' && componentAutodocs);
// We need a docs entry attached to the CSF file if either:
// a) it is a stories.mdx transpiled to CSF, OR
// b) we have docs page enabled for this file
if (componentTags.includes(STORIES_MDX_TAG) || autodocsOptedIn) {
const name = this.options.docs.defaultName;
const id = toId(csf.meta.title, name);
entries.unshift({
id,
title: csf.meta.title,
name,
importPath,
type: 'docs',
tags: [
...componentTags,
'docs',
...(autodocsOptedIn && !componentAutodocs ? [AUTODOCS_TAG] : []),
],
storiesImports: [],
});
}
const componentTags = csf.meta.tags || [];
csf.stories.forEach(({ id, name, tags: storyTags, parameters }) => {
if (!parameters?.docsOnly) {
const tags = [...(storyTags || componentTags), 'story'];
entries.push({ id, title: csf.meta.title, name, importPath, tags, type: 'story' });
}
} catch (err) {
if (err instanceof NoMetaError) {
logger.info(`💡 Skipping ${relativePath}: ${err}`);
} else {
logger.warn(`🚨 Extraction error on ${relativePath}: ${err}`);
throw err;
});

if (csf.stories.length) {
const { autodocs } = this.options.docs;
const componentAutodocs = componentTags.includes(AUTODOCS_TAG);
const autodocsOptedIn = autodocs === true || (autodocs === 'tag' && componentAutodocs);
// We need a docs entry attached to the CSF file if either:
// a) it is a stories.mdx transpiled to CSF, OR
// b) we have docs page enabled for this file
if (componentTags.includes(STORIES_MDX_TAG) || autodocsOptedIn) {
const name = this.options.docs.defaultName;
const id = toId(csf.meta.title, name);
entries.unshift({
id,
title: csf.meta.title,
name,
importPath,
type: 'docs',
tags: [
...componentTags,
'docs',
...(autodocsOptedIn && !componentAutodocs ? [AUTODOCS_TAG] : []),
],
storiesImports: [],
});
}
}

return { entries, type: 'stories', dependents: [] } as StoriesCacheEntry;
}

Expand Down Expand Up @@ -381,7 +372,6 @@ export class StoryIndexGenerator {
};
return docsEntry;
} catch (err) {
logger.warn(`🚨 Extraction error on ${chalk.blue(relativePath)}: ${err}`);
if (err.source?.match(/mdast-util-mdx-jsx/g)) {
logger.warn(
`💡 This seems to be an MDX2 syntax error. Please refer to the MDX section in the following resource for assistance on how to fix this: ${chalk.yellow(
Expand All @@ -407,9 +397,9 @@ export class StoryIndexGenerator {

// This shouldn't be possible, but double check and use for typing
if (worseEntry.type === 'story')
throw new DuplicateEntriesError(`Duplicate stories with id: ${firstEntry.id}`, [
firstEntry,
secondEntry,
throw new IndexingError(`Duplicate stories with id: ${firstEntry.id}`, [
firstEntry.importPath,
secondEntry.importPath,
]);

if (betterEntry.type === 'story') {
Expand All @@ -435,8 +425,9 @@ export class StoryIndexGenerator {

// If you link a file to a tagged CSF file, you have probably made a mistake
if (worseEntry.tags?.includes(AUTODOCS_TAG) && this.options.docs.autodocs !== true)
throw new Error(
`You created a component docs page for ${worseEntry.title} (${betterEntry.importPath}), but also tagged the CSF file (${worseEntry.importPath}) with '${AUTODOCS_TAG}'. This is probably a mistake.`
throw new IndexingError(
`You created a component docs page for '${worseEntry.title}', but also tagged the CSF file with '${AUTODOCS_TAG}'. This is probably a mistake.`,
[betterEntry.importPath, worseEntry.importPath]
);

// Otherwise the existing entry is created by `autodocs=true` which allowed to be overridden.
Expand All @@ -458,18 +449,7 @@ export class StoryIndexGenerator {
return betterEntry;
}

async sortStories(storiesList: IndexEntry[]) {
const entries: StoryIndex['entries'] = {};

storiesList.forEach((entry) => {
const existing = entries[entry.id];
if (existing) {
entries[entry.id] = this.chooseDuplicate(existing, entry);
} else {
entries[entry.id] = entry;
}
});

async sortStories(entries: StoryIndex['entries']) {
const sortableStories = Object.values(entries);

// Skip sorting if we're in v6 mode because we don't have
Expand All @@ -495,10 +475,27 @@ export class StoryIndexGenerator {
const storiesList = await this.ensureExtracted();

try {
const firstError = storiesList.find((entry) => entry.type === 'error');
if (firstError) throw (firstError as ErrorEntry).err;
const errorEntries = storiesList.filter((entry) => entry.type === 'error');
if (errorEntries.length)
throw new MultipleIndexingError(errorEntries.map((entry) => (entry as ErrorEntry).err));

const duplicateErrors: IndexingError[] = [];
const indexEntries: StoryIndex['entries'] = {};
(storiesList as IndexEntry[]).forEach((entry) => {
try {
const existing = indexEntries[entry.id];
if (existing) {
indexEntries[entry.id] = this.chooseDuplicate(existing, entry);
} else {
indexEntries[entry.id] = entry;
}
} catch (err) {
duplicateErrors.push(err);
}
});
if (duplicateErrors.length) throw new MultipleIndexingError(duplicateErrors);

const sorted = await this.sortStories(storiesList as IndexEntry[]);
const sorted = await this.sortStories(indexEntries);

let compat = sorted;
if (this.options.storiesV2Compatibility) {
Expand Down Expand Up @@ -534,8 +531,7 @@ export class StoryIndexGenerator {
return this.lastIndex;
} catch (err) {
this.lastError = err;
logger.warn(`🚨 Couldn't fetch index`);
logger.warn(this.lastError.stack);
logger.warn(`🚨 ${this.lastError.toString()}`);
throw this.lastError;
}
}
Expand Down
Loading

0 comments on commit 35ece38

Please sign in to comment.