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

Initial support for @storybook/server #9250

Merged
merged 892 commits into from
Feb 4, 2020

Conversation

jonspalmer
Copy link
Contributor

Issue: Added a new "server" framework

What I did

Added @storybook/server based on @storybook/html.
What's new is that the configure method takes a new fetchHtml argument that is responsible for fetching the html from the story.

How to test

I will followup with a kitchen sink example.

Usage is something like this:

import { configure } from '@storybook/server';
import Stories from '../someStorySource';

const fetchHtml = async function(id, params) {
  const url = new URL('http://localhost:3000/storybook_preview_show');
  url.search = new URLSearchParams({id, ...params} ).toString();

  const response = await fetch(url);
  return  await response.text();
}

configure(fetchHtml, () => Stories, module);

Open Questions/Issues

  • My Typescript foo is weak. Could do with some 👀to make sure I got it right
  • Tests? There don't seem to be tests for frameworks but maybe I'm missing something.
  • Async Story loading? What's the right way to handle this? Probably needs first class support in core.

@vercel
Copy link

vercel bot commented Dec 26, 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/527hm46c9
✅ Preview: https://monorepo-git-fork-bostanio-storybookserver.storybook.now.sh

@shilman shilman added this to the 6.0.0 milestone Dec 26, 2019
@vercel vercel bot temporarily deployed to Preview December 26, 2019 17:01 Inactive
@shilman
Copy link
Member

shilman commented Dec 26, 2019

Amazing progress @bostanio! I'm guessing this will take some back and forth to get right, so I'll start things off:

  1. Each framework has at least one example in examples/ which provides an end-to-end test. This def needs one!

  2. I don't think configure should take an extra argument. How about providing a separate function to configure it? (Yes, I get that making it required in configure helps you enforce that it's provided)

import { setFetchHtml } from '@storybook/server';
setFetchHtml(someFunction);

@jonspalmer
Copy link
Contributor Author

@shilman Working on an example. Exploring the simplest 'app' to use as the backend. I'm guessing its express.

Can certainly change the configure signature. I like forcing it in configure given its required but I can see the value in all frameworks having the same signature.

@shilman
Copy link
Member

shilman commented Dec 27, 2019

I'd also be open to a third options object argument to configure.

@jonspalmer
Copy link
Contributor Author

I'd also be open to a third options object argument to configure.

OK I'll go with that.

@vercel vercel bot temporarily deployed to Preview December 27, 2019 02:12 Inactive
@jonspalmer
Copy link
Contributor Author

Added a first pass at the example app. Would love some advice on how we might structure that example and how much of a 'kitchen sink' we want it to be.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

This is so cool!

app/server/src/client/preview/render.ts Outdated Show resolved Hide resolved
app/server/src/server/framework-preset-server.ts Outdated Show resolved Hide resolved
app/server/src/server/build.ts Show resolved Hide resolved
@tmeasday
Copy link
Member

tmeasday commented Feb 3, 2020

@jonspalmer there are still two unresolved comments on that PR that we need to sort out:

  1. Is this a mistake or did I misunderstand: Initial support for @storybook/server #9250 (comment)

  2. Do we need to deal with this TODO: https://github.com/storybookjs/storybook/pull/9250/files/be4d06c0cd69b2ed4a79ad390852770bfd6ccfeb#diff-83aae226cfb73b52d12dfd37ae15a411?

Also a change to mdx.tsx got dropped because generateHrefWithHash doesn't exist any more. I'm not sure if that's going to be a problem but I'll have a proper play with this once the above two questions are resolved.

@ndelangen
Copy link
Member

@shilman I assume you got a ok from @tmeasday to merge? Looks like he still had a few concerns?

@shilman
Copy link
Member

shilman commented Feb 4, 2020

@ndelangen yeah he filed some follow-up issues

@tmeasday
Copy link
Member

tmeasday commented Feb 5, 2020

@ndelangen this was a bit confusing but @jonspalmer answered my questions on the other PR

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.

None yet