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

Source-loader: Warn if applied to non-stories file #8773

Merged
merged 3 commits into from
Nov 11, 2019

Conversation

donaldpipowitch
Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Nov 9, 2019

Issue: #8294

What I did

I added a warning for people like me who misconfigured the @storybook/source-loader. If I understand correctly, it should only be used for files containing stories. In my webpack config it was applied to all source code files.

How to test

Checkout my PR, cd lib/source-loader && yarn prepare && yarn link. After that $ git clone [email protected]:donaldpipowitch/storybook-bug-demo-source-loader-context-null.git && cd storybook-bug-demo-source-loader-context-null && yarn && yarn link "@storybook/source-loader" && yarn storybook. You should see the warning.

image

@vercel
Copy link

vercel bot commented Nov 9, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/l2lrut76i
🌍 Preview: https://monorepo-git-fork-donaldpipowitch-fix-8294-source-loader.storybook.now.sh

@shilman shilman changed the title fix 8294: check context in source-loader Source-loader: Warn if applied to non-story file Nov 9, 2019
@shilman shilman changed the title Source-loader: Warn if applied to non-story file Source-loader: Warn if applied to non-stories file Nov 9, 2019
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks @donaldpipowitch, this is great! Mind updating this to use @storybook/node-logger instead?

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Nov 9, 2019
@donaldpipowitch
Copy link
Contributor Author

@shilman sure :) Thank you for the fast review.

@donaldpipowitch
Copy link
Contributor Author

Mind updating this to use @storybook/node-logger instead?

@shilman did you meant @storybook/client-logger by any chance? Because I changed lib/source-loader/src/client/preview.js (inside client).

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@donaldpipowitch You're right, it's client-logger

@donaldpipowitch
Copy link
Contributor Author

@shilman Fixed 👍

@shilman shilman merged commit 52a5dc5 into storybookjs:next Nov 11, 2019
@donaldpipowitch donaldpipowitch deleted the fix-8294-source-loader branch November 11, 2019 10:50
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Dec 2, 2019
shilman added a commit that referenced this pull request Dec 2, 2019
Source-loader: Warn if applied to non-stories file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 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 source-loader
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants