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.

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

CSF3: Handle auto-title redundant filename #17421

Merged
merged 2 commits into from
Feb 4, 2022
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
19 changes: 19 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<h1>Migration</h1>

- [From version 6.4.x to 6.5.0](#from-version-64x-to-650)
- [CSF3 auto-title redundant filename](#csf3-auto-title-redundant-filename)
- [From version 6.3.x to 6.4.0](#from-version-63x-to-640)
- [Automigrate](#automigrate)
- [CRA5 upgrade](#cra5-upgrade)
Expand Down Expand Up @@ -188,6 +190,23 @@
- [Packages renaming](#packages-renaming)
- [Deprecated embedded addons](#deprecated-embedded-addons)

## From version 6.4.x to 6.5.0

### CSF3 auto-title redundant filename

SB 6.4 introduced experimental "auto-title", in which a story's location in the sidebar (aka `title`) can be automatically inferred from its location on disk. For example, the file `atoms/Button.stories.js` might result in the title `Atoms/Button`.

The heuristic failed in the common scenario in which each component gets its own directory, e.g. `atoms/Button/Button.stories.js`, which would result in the redundant title `Atoms/Button/Button`. Alternatively, `atoms/Button/index.stories.js` would result in `Atoms/Button/Index`.

To address this problem, 6.5 introduces a new heuristic to removes the filename if it matches the directory name (case insensitive) or `index`. So `atoms/Button/Button.stories.js` and `atoms/Button/index.stories.js` would both result in the title `Atoms/Button`.

Since CSF3 is experimental, we are introducing this technically breaking change in a minor release. If you desire the old structure, you can manually specify the title in file. For example:

```js
// atoms/Button/Button.stories.js
export default { title: 'Atoms/Button/Button' };
```

## From version 6.3.x to 6.4.0

### Automigrate
Expand Down
18 changes: 18 additions & 0 deletions lib/store/src/autoTitle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,24 @@ describe('autoTitle', () => {
).toMatchInlineSnapshot(`Atoms/To/File`);
});

it('match with trailing duplicate', () => {
expect(
auto(
'./path/to/button/button.stories.js',
normalizeStoriesEntry({ directory: './path' }, options)
)
).toMatchInlineSnapshot(`To/Button`);
});

it('match with trailing index', () => {
expect(
auto(
'./path/to/button/index.stories.js',
normalizeStoriesEntry({ directory: './path' }, options)
)
).toMatchInlineSnapshot(`To/Button`);
});

Copy link
Member

Choose a reason for hiding this comment

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

What's the outcome of a test with this?

auto(
  './path/to/button/index.stories.js',
  normalizeStoriesEntry({ directory: './path' }, options)
)

Copy link
Member Author

Choose a reason for hiding this comment

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

To/Button/Index -- do you think we should remove Index?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say so, normally index is ommited when importing e.g.

import { Button } from './components/Button'

so personally I'd expect a similar behavior. @tmeasday @ndelangen WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, "Index" is meaningless to everyone, we should ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks gang! 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Do we also cope with just stories.js? I feel like that filename would make sense for a folder.

Copy link
Member

@yannbf yannbf Feb 6, 2022

Choose a reason for hiding this comment

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

There was an issue in the eslint-plugin-storybook repo about supporting stories.js files. We never suggest that though, and I believe our internal defaults are all about *.stories.*. Do you think we should support stories.* ? I've never seen people having something like test.js in their folders, just index.test.js or Component.test.js

Copy link
Member

Choose a reason for hiding this comment

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

I've seen it before, but I'm OK with saying it's not something we recommend.

it('match with hyphen path', () => {
expect(
auto(
Expand Down
23 changes: 17 additions & 6 deletions lib/store/src/autoTitle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ interface NormalizedStoriesSpecifier {
importPathMatcher: RegExp;
}

const stripExtension = (titleWithExtension: string) => {
let parts = titleWithExtension.split('/');
const stripExtension = (path: string[]) => {
let parts = [...path];
const last = parts[parts.length - 1];
const dotIndex = last.indexOf('.');
const stripped = dotIndex > 0 ? last.substr(0, dotIndex) : last;
Expand All @@ -20,11 +20,19 @@ const stripExtension = (titleWithExtension: string) => {
if (first === '') {
parts = rest;
}
return parts.join('/');
return parts;
};

const startCaseTitle = (title: string) => {
return title.split('/').map(startCase).join('/');
// deal with files like "atoms/button/{button,index}.stories.js"
const removeRedundantFilename = (paths: string[]) => {
let prevVal: string;
return paths.filter((val, index) => {
if (index === paths.length - 1 && (val === prevVal || val === 'Index')) {
return false;
}
prevVal = val;
return true;
});
};

/**
Expand All @@ -48,7 +56,10 @@ export const autoTitleFromSpecifier = (fileName: string, entry: NormalizedStorie
if (importPathMatcher.exec(normalizedFileName)) {
const suffix = normalizedFileName.replace(directory, '');
const titleAndSuffix = slash(pathJoin([titlePrefix, suffix]));
return startCaseTitle(stripExtension(titleAndSuffix));
let path = titleAndSuffix.split('/');
path = stripExtension(path).map(startCase);
path = removeRedundantFilename(path);
return path.join('/');
}
return undefined;
};
Expand Down