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

enable writing api documentation #3010

Closed
wants to merge 2 commits into from
Closed

enable writing api documentation #3010

wants to merge 2 commits into from

Conversation

walkerrandolphsmith
Copy link

Issue:

#591 (comment)

What I did

I wanted to change what route storybook runs on and attempted to accomplish this with the following "full control" configuration.

const genDefaultConfig = require('@storybook/react/dist/server/config/defaults/webpack.config.js');

module.exports = (baseConfig, env) => {
  const storybookBaseConfig = genDefaultConfig(baseConfig, env);

  storybookBaseConfig.entry.preview = storybookBaseConfig.entry.preview.map(function(entry) {
    if (entry.includes("webpack-hot-middleware")) {
      return `${require.resolve('webpack-hot-middleware/client')}?path=/book/__webpack_hmr&reload=true`;
    }

    return entry;
  });

  storybookBaseConfig.output.publicPath = '/book/';

  return storybookBaseConfig;
};

As mentioned in the issue comment I linked, the webpack hot middleware was not receiving the configuration it needed to enable a custom route.

I constructed a minimal configuration for webpack hot middleware including the path.

How to test

Use a "full control" config as described and the default config and ensure HMR works for both storybooks

Is this testable with jest or storyshots?
Not sure

Does this need a new example in the kitchen sink apps?
I don't think so its not a common case.

Does this need an update to the documentation?
Possibly not since this is an edge case.

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

@Hypnosphi Hypnosphi added configuration babel / webpack core bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Feb 17, 2018
@Hypnosphi Hypnosphi self-requested a review February 17, 2018 15:57
@Hypnosphi
Copy link
Member

Thanks, will test

@Hypnosphi
Copy link
Member

Hypnosphi commented Feb 17, 2018

Actually, modifying entry and output fields doesn't sound like a good idea to me (actually docs advise not to do that)

I wonder can there be another way to achieve what you need

@Hypnosphi Hypnosphi added feature request and removed bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Feb 17, 2018
@Hypnosphi
Copy link
Member

Hypnosphi commented Feb 17, 2018

I think we can make publicPath explicitly configurable with a CLI option. How do you feel about adding that?

@walkerrandolphsmith
Copy link
Author

I will investigate other solutions! Thanks @Hypnosphi

@walkerrandolphsmith
Copy link
Author

That suggestion makes sense let me try that path!

@Hypnosphi
Copy link
Member

By the way, we use publicPath: '' for static builds to allow deploying into subpaths. Maybe this can work for devserver as well?

@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #3010 into master will decrease coverage by 55.14%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3010       +/-   ##
===========================================
- Coverage    92.5%   37.35%   -55.15%     
===========================================
  Files           6      426      +420     
  Lines          40     9150     +9110     
  Branches        2      869      +867     
===========================================
+ Hits           37     3418     +3381     
- Misses          2     5205     +5203     
- Partials        1      527      +526
Impacted Files Coverage Δ
lib/core/src/server/middleware.js 0% <0%> (ø)
addons/info/src/components/Story.js 88.03% <0%> (ø)
lib/ui/src/modules/ui/components/shortcuts_help.js 12.85% <0%> (ø)
addons/a11y/src/register.js 0% <0%> (ø)
addons/jest/src/components/Result.js 0% <0%> (ø)
.../ui/src/modules/ui/components/menu_item.stories.js 100% <0%> (ø)
...dons/actions/src/lib/types/object/getObjectName.js 62.5% <0%> (ø)
...mponents/stories_panel/stories_tree/tree_header.js 18.75% <0%> (ø)
addons/info/src/components/types/PropertyLabel.js 78.94% <0%> (ø)
app/vue/src/server/config/babel.js 100% <0%> (ø)
... and 411 more

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 44d2293...9851095. Read the comment docs.

@Hypnosphi Hypnosphi added this to the v4.0.0 milestone Mar 8, 2018
@walkerrandolphsmith
Copy link
Author

Hey I am in the middle of moving so a bit busy. I see this was added to v4 milestone! Super excited to make it happen.

@stale
Copy link

stale bot commented Mar 31, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Mar 31, 2018
@stale
Copy link

stale bot commented Apr 30, 2018

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

@stale stale bot closed this Apr 30, 2018
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