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

Addon-storysource: Replace loader with source-loader ✂ #7272

Merged
merged 12 commits into from
Jul 8, 2019

Conversation

libetl
Copy link
Member

@libetl libetl commented Jul 3, 2019

Issue: #7192

What I did

I have removed the storysource older loader
Also the new source-loader is not yet compatible with storysource V1 : fixed

How to test

  • Is this testable with Jest or Chromatic screenshots? no
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? updpated

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Jul 3, 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-task-i-have-removed-storysource-loader.storybook.now.sh

@@ -1 +1 @@
module.exports = require('./dist/loader');
module.exports = require('@storybook/source-loader');
Copy link
Member Author

Choose a reason for hiding this comment

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

for backward compatibility, but it will be recommended to change the usage in webpack of course.

@@ -35,7 +35,8 @@
"regenerator-runtime": "^0.12.1"
},
"peerDependencies": {
"react": "*"
"react": "*",
"@storybook/source-loader": "*"
Copy link
Member Author

Choose a reason for hiding this comment

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

we will ask the developer to pull the source-loader separately

Copy link
Member

@Hypnosphi Hypnosphi Jul 20, 2019

Choose a reason for hiding this comment

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

@shilman @ndelangen should we add this line to our docs?

Please don't use caret when depending on Storybook packages, as we regularly do breaking changes in minor releases

@shilman
Copy link
Member

shilman commented Jul 4, 2019

@libetl Thanks so much for putting this together. It's looking pretty good. Some testing notes:

  • I added source-loader to vue-kitchen-sink
  • As I tested it out (in official-storybook and vue-kitchen-sink), I found there are two kinds of stories in the module format:
// single-word name
export const single = () => ...

// multi-word name
export const multiWord = () => ...
multiWord.story = { name: 'multi word' }

Single-word seems to work in both storysource and also in docs (view docs, and open "view source").

Multi-word seems to be broken in the current PR.

EDIT: I've fixed the problem

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.

Magnifique! 👌

@shilman shilman changed the title #7192 ✂ I have removed the storysource older loader Addon-storysource: ✂ Replace old loader with source-loader Jul 4, 2019
@shilman shilman changed the title Addon-storysource: ✂ Replace old loader with source-loader Addon-storysource: ✂ Replace loader with source-loader Jul 4, 2019
@shilman shilman added this to the 5.2.0 milestone Jul 4, 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.

@libetl Ok, it looks like build-storybook in examples/angular-cli is taking forever in CI. I also see this behavior running on my dev machine.

This brings up a few thoughts:

  1. Is `parser: 'typescript' even necessary with the new loader?
  2. Can we make the following options defaults:
          injectParameters: true,
          inspectLocalDependencies: false,
          inspectDependencies: false,
  1. Why isn't angular-cli building, even when I set those options? I get an out of memory error, even updating include: [resolve(__dirname, '../src/stories')]

@shilman shilman changed the title Addon-storysource: ✂ Replace loader with source-loader Addon-storysource: Replace loader with source-loader ✂ Jul 4, 2019
@libetl
Copy link
Member Author

libetl commented Jul 5, 2019

InjectDependencies and injectLocalDependencies default to false of course

@shilman
Copy link
Member

shilman commented Jul 6, 2019

@libetl did you have a chance to look at the running times? It also results in an out of memory error on my local machine. Not mergeable yet.

@shilman shilman mentioned this pull request Jul 7, 2019
@shilman
Copy link
Member

shilman commented Jul 8, 2019

Per your suggestion I've verified that the line var withSourceLoader = require('@storybook/source-loader').withSource; causes the build to blow up on my dev machine.

Here's my proposal:

  1. Make injectParameters true (and not user-configurable)
  2. Rewrite storysource to use parameters instead of events Storysource: Use story parameters injected by source-loader #7192
  3. Remove injectDecorators entirely -- we don't need back-compat
  4. Make sure none of the generated code includes additional imports since that seems to be causing problems

@libetl
Copy link
Member Author

libetl commented Jul 8, 2019

Hmm. Do you want that for 5.2 ? (Just to clarify)

@shilman
Copy link
Member

shilman commented Jul 8, 2019

@libetl Ideally. I just need source-loader to work for 5.2 with both storysource & docs and I think this is the most clean and direct way to do it. I can do it if you're busy.

@libetl libetl requested review from danielduan and usulpro as code owners July 8, 2019 13:10
@vercel vercel bot temporarily deployed to staging July 8, 2019 13:10 Inactive
@github-actions
Copy link
Contributor

Fails
🚫 PR is marked with "BREAKING CHANGE" label.
🚫

Please choose only one of these labels: ["BREAKING CHANGE","bug"]

Generated by 🚫 dangerJS against c50916e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants