Skip to content
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.

Sign up for GitHub

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

TestBuild: Fix indexer bug #24890

Merged
merged 8 commits into from
Nov 17, 2023
Merged

TestBuild: Fix indexer bug #24890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8a377f8
make it work with --test
ndelangen Nov 17, 2023
dc4786c
modify the default_files_pattern for normalizeStories to exclude mdx …
ndelangen Nov 17, 2023
861170e
cleanup
ndelangen Nov 17, 2023
c643d59
fix
ndelangen Nov 17, 2023
36d0747
ensure the changed default_files_pattern is applied to entries that d…
ndelangen Nov 17, 2023
07374cc
disable locally
ndelangen Nov 17, 2023
ddbb446
simpler code
ndelangen Nov 17, 2023
ffb9003
Merge branch 'next' into norbert/improve-mdx-filtering
ndelangen Nov 17, 2023
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
9 changes: 5 additions & 4 deletions code/lib/core-common/src/utils/normalize-stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { normalizeStoryPath } from './paths';
import { globToRegexp } from './glob-to-regexp';

const DEFAULT_TITLE_PREFIX = '';
const DEFAULT_FILES = '**/*.@(mdx|stories.@(js|jsx|mjs|ts|tsx))';
const DEFAULT_FILES_PATTERN = '**/*.@(mdx|stories.@(js|jsx|mjs|ts|tsx))';

const isDirectory = (configDir: string, entry: string) => {
try {
Expand All @@ -34,7 +34,7 @@ export const getDirectoryFromWorkingDir = ({

export const normalizeStoriesEntry = (
entry: StoriesEntry,
{ configDir, workingDir }: NormalizeOptions
{ configDir, workingDir, default_files_pattern = DEFAULT_FILES_PATTERN }: NormalizeOptions
Copy link
Member

Choose a reason for hiding this comment

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

Why snake case when the other options are camel case?

): NormalizedStoriesSpecifier => {
let specifierWithoutMatcher: Omit<NormalizedStoriesSpecifier, 'importPathMatcher'>;

Expand All @@ -53,7 +53,7 @@ export const normalizeStoriesEntry = (
specifierWithoutMatcher = {
titlePrefix: DEFAULT_TITLE_PREFIX,
directory: entry,
files: DEFAULT_FILES,
files: default_files_pattern,
};
} else {
specifierWithoutMatcher = {
Expand All @@ -65,7 +65,7 @@ export const normalizeStoriesEntry = (
} else {
specifierWithoutMatcher = {
titlePrefix: DEFAULT_TITLE_PREFIX,
files: DEFAULT_FILES,
files: default_files_pattern,
...entry,
};
}
Expand Down Expand Up @@ -99,6 +99,7 @@ export const normalizeStoriesEntry = (
interface NormalizeOptions {
configDir: string;
workingDir: string;
default_files_pattern?: string;
}

export const normalizeStories = (entries: StoriesEntry[], options: NormalizeOptions) => {
Expand Down
57 changes: 41 additions & 16 deletions code/lib/core-server/src/presets/common-override-preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import type { Options, PresetProperty, StorybookConfig, TestBuildFlags } from '@storybook/types';
import type {
Options,
PresetProperty,
StoriesEntry,
StorybookConfig,
TestBuildFlags,
} from '@storybook/types';
import { normalizeStories, commonGlobOptions } from '@storybook/core-common';
import { isAbsolute, join } from 'path';
import { isAbsolute, join, relative } from 'path';
import slash from 'slash';
import { glob } from 'glob';

Expand All @@ -19,30 +25,49 @@ export const framework: PresetProperty<'framework', StorybookConfig> = async (co

export const stories: PresetProperty<'stories', StorybookConfig> = async (entries, options) => {
if (options?.build?.test?.disableMDXEntries) {
return (
const list = normalizeStories(entries, {
configDir: options.configDir,
workingDir: options.configDir,
default_files_pattern: '**/*.@(stories.@(js|jsx|mjs|ts|tsx))',
});
const result = (
await Promise.all(
normalizeStories(entries, {
configDir: options.configDir,
workingDir: options.configDir,
}).map(({ directory, files }) => {
list.map(async ({ directory, files, titlePrefix }) => {
const pattern = join(directory, files);
const absolutePattern = isAbsolute(pattern) ? pattern : join(options.configDir, pattern);
const absoluteDirectory = isAbsolute(directory)
? directory
: join(options.configDir, directory);

return glob(slash(absolutePattern), {
...commonGlobOptions(absolutePattern),
follow: true,
});
return {
files: (
await glob(slash(absolutePattern), {
...commonGlobOptions(absolutePattern),
follow: true,
})
).map((f) => relative(absoluteDirectory, f)),
directory,
titlePrefix,
};
})
)
).flatMap((expanded, i) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't using a flatMap a lot simpler?

const filteredEntries = expanded.filter((s) => !s.endsWith('.mdx'));
).flatMap<StoriesEntry>((expanded, i) => {
const filteredEntries = expanded.files.filter((s) => !s.endsWith('.mdx'));
// only return the filtered entries when there is something to filter
// as webpack is faster with unexpanded globs
if (filteredEntries.length < expanded.length) {
return filteredEntries;
let items = [];
if (filteredEntries.length < expanded.files.length) {
items = filteredEntries.map((k) => ({
...expanded,
files: `**/${k}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the part I'm not really happy about... But it was the only solution I could really find without digging into this:

storybook/code/lib/core-common/src/utils/glob-to-regexp.ts

Lines 3 to 27 in ff6b28e

export function globToRegexp(glob: string) {
const regex = pico.makeRe(glob, {
fastpaths: false,
noglobstar: false,
bash: false,
});
if (!regex.source.startsWith('^')) {
throw new Error(`Invalid glob: >> ${glob} >> ${regex}`);
}
if (!glob.startsWith('./')) {
return regex;
}
// makeRe is sort of funny. If you pass it a directory starting with `./` it
// creates a matcher that expects files with no prefix (e.g. `src/file.js`)
// but if you pass it a directory that starts with `../` it expects files that
// start with `../`. Let's make it consistent.
// Globs starting `**` require special treatment due to the regex they
// produce, specifically a negative look-ahead
return new RegExp(
['^\\.', glob.startsWith('./**') ? '' : '[\\\\/]', regex.source.substring(1)].join('')
);
}

... and it really do not want to go digging into that code, right now.

I'm thinking this is "close enough" for now.

Copy link
Member

Choose a reason for hiding this comment

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

It would be a lot easier to understand what this code was doing if we had a test for it.

I am assuming it turns something like:

[{ directory: 'foo', files: '**/*.(mdx|stories.js)' }]

Into something like:

[
  { directory: 'foo', files: '**/first-story.stories.js' },
  { directory: 'foo', files: '**/second-story.stories.js' },
  //..
]

Where that list can be very long? That seems like it would have a pretty detritmental impact on build times if that list was really long, but I guess we would have to test it it out to know for sure.

I wonder if there's a way to hit the most common cases and just drop the mdx| bit from the regexp, if we see a common pattern like '**/*.@(mdx|stories.@(js|jsx|mjs|ts|tsx))';

Also, what if the expanded list is empty?

}));
} else {
items = [list[i]];
}
return entries[i];

return items;
});
return result;
}
return entries;
};
Expand Down
6 changes: 6 additions & 0 deletions code/ui/.storybook/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const isBlocksOnly = process.env.STORYBOOK_BLOCKS_ONLY === 'true';
const allStories = [
{
directory: '../manager/src',
files: '**/*.stories.@(js|jsx|mjs|ts|tsx|mdx)',
titlePrefix: '@manager',
},
{
Expand Down Expand Up @@ -53,6 +54,11 @@ const config: StorybookConfig = {
'@storybook/addon-designs',
'@chromaui/addon-visual-tests',
],
build: {
test: {
disableBlocks: false,
},
},
Comment on lines +57 to +61
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed because of:

storybook/code/ui/.storybook/preview.tsx

Lines 141 to 159 in ab7fd6d

(Story, { loaded: { docsContext } }) =>
docsContext ? (
<DocsContext.Provider value={docsContext}>
<Story />
</DocsContext.Provider>
) : (
<Story />
),
/**
* This decorator adds wrappers that contains global styles for stories to be targeted by.
* Activated with parameters.docsStyles = true
*/ (Story, { parameters: { docsStyles } }) =>
docsStyles ? (
<DocsPageWrapper>
<Story />
</DocsPageWrapper>
) : (
<Story />
),

This breaks at runtime, because if blocks is replaced as a module by an empty object, that object (obviously) does not contain what is needed here.

framework: {
name: '@storybook/react-vite',
options: {},
Expand Down
143 changes: 0 additions & 143 deletions code/ui/components/src/components/typography/DocumentFormattingSample.mdx
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions code/ui/components/src/components/typography/DocumentWrapper.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@
import React from 'react';

import { DocumentWrapper } from './DocumentWrapper';
import MarkdownSample from './DocumentFormattingSample.mdx';

export default {
component: DocumentWrapper,
decorators: [(storyFn: any) => <div style={{ width: '600px' }}>{storyFn()}</div>],
};

export const WithMarkdown = () => (
<DocumentWrapper>
<MarkdownSample />
</DocumentWrapper>
);
ndelangen marked this conversation as resolved.
Show resolved Hide resolved

export const WithDOM = () => (
<DocumentWrapper>
<h1>h1 Heading</h1>
Expand Down