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

Fix infinite loop with special characters in kind names #6607

Merged
merged 8 commits into from
Apr 26, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Apr 24, 2019

Issue: #6128

What I did

Detect when there are pathological kind names, e.g. 'Vue <docs/>' where / is a path separator and > is a special character that gets filtered out from Story id's.

Throw an error and warn the user rather then the "Maximum call stack error".

I also did a little refactoring.

How to test

Create a story in official-storybook:

storiesOf('Something <docs />', module)
  .add('something', () => <Button />)

Before the change, this will mysteriously throw the Maximum call stack error. After, it will throw the error:

Invalid kind with id === parentId: 'something-docs'

Did you create a path that uses the separator char accidentally, such as 'Vue <docs/>' where '/' is a separator char? See https://github.com/storybooks/storybook/issues/6128

@shilman shilman added bug patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Apr 24, 2019
@vercel
Copy link

vercel bot commented Apr 24, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-6128-fix-kind-sanitization-error.storybook.now.sh

@shilman shilman added this to the 5.0.x milestone Apr 24, 2019
export const knownNonViewModesRegex = /(settings)/;
const splitPath = /\/([^/]+)\/([^/]+)?/;
const splitPathRegex = /\/([^/]+)\/([^/]+)?/;
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this to something else given it has nothing to do with the splitPath function?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tmeasday Renamed the functions instead. LMK what you think.

Copy link
Member

Choose a reason for hiding this comment

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

WFM 👍

lib/api/src/modules/stories.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging April 24, 2019 10:21 Inactive
@codecov
Copy link

codecov bot commented Apr 24, 2019

Codecov Report

Merging #6607 into next will decrease coverage by 0.04%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6607      +/-   ##
==========================================
- Coverage   40.67%   40.62%   -0.05%     
==========================================
  Files         633      633              
  Lines        8674     8672       -2     
  Branches      620      621       +1     
==========================================
- Hits         3528     3523       -5     
- Misses       5057     5059       +2     
- Partials       89       90       +1
Impacted Files Coverage Δ
lib/client-api/src/story_store.js 76.31% <ø> (+1.73%) ⬆️
lib/router/src/router.tsx 0% <0%> (ø) ⬆️
lib/api/src/modules/stories.ts 80.8% <33.33%> (-2.53%) ⬇️
lib/router/src/utils.ts 47.22% <37.5%> (-4.4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a71a0b...dabc161. Read the comment docs.

@@ -1,4 +1,5 @@
import { toId, sanitize } from '@storybook/router/dist/utils';
// FIXME: we shouldn't import from dist but there are no types otherwise
import { toId, sanitize, parseKind } from '@storybook/router/dist/utils';
Copy link
Member

Choose a reason for hiding this comment

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

@shilman you can do:
import { toId, sanitize, parseKind } from '@storybook/router';

because index.ts of @storybook/router is exporting everything from /utils folder:

export * from './utils';
export * from './router';

export const knownNonViewModesRegex = /(settings)/;
const splitPath = /\/([^/]+)\/([^/]+)?/;
const splitPathRegex = /\/([^/]+)\/([^/]+)?/;
Copy link
Member

Choose a reason for hiding this comment

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

WFM 👍

@shilman shilman merged commit a85decf into next Apr 26, 2019
@shilman shilman deleted the 6128-fix-kind-sanitization-error branch April 26, 2019 09:36
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants