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-docs: Source loader library #7117

Merged
merged 10 commits into from Jun 25, 2019
Merged

Addon-docs: Source loader library #7117

merged 10 commits into from Jun 25, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 18, 2019

Issue: #7101

What I did

This is the source loader library by @libetl that replaces addon-storysource's loader and extends it in various ways:

  • storiesOf support
  • module story format
  • Typescript support
  • Flow support

In addition, it can also embed the story source as a story parameter (which is probably how it should be implemented).

I'll leave the refactor of addon-storysource to @libetl for another PR. It might even be worth updating storysource to use the parameter and removing the complicated decorator/event.

This will be integrated into docs in a subsequent PR where the Source doc block consumes the aforementioned story parameter.

How to test

yarn test

@vercel
Copy link

vercel bot commented Jun 18, 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-7101-source-loader.storybook.now.sh

@shilman shilman changed the title Source loader library by @libetl Addon-docs: Source loader library Jun 18, 2019
@igor-dv
Copy link
Member

igor-dv commented Jun 18, 2019

@shilman , I don't get why not to reference it in the storysource ? since it looks like a copy paste, hard to understand what exactly changed =) But It looks similar to what I remember it was =)

@shilman
Copy link
Member Author

shilman commented Jun 18, 2019

@igor-dv see my comment above:

I'll leave the refactor of addon-storysource to @libetl for another PR. It might even be worth updating storysource to use the parameter and removing the complicated decorator/event.

@libetl
Copy link
Member

libetl commented Jun 19, 2019

@igor-dv you guessed it, storysource will just require it.
The idea behind it is to share the implementation

@igor-dv
Copy link
Member

igor-dv commented Jun 19, 2019

👍

@shilman
Copy link
Member Author

shilman commented Jun 19, 2019

@libetl With the latest commit I've updated all the monorepo examples to use source-loader instead of storysource's loader. However, storysource appears to be broken in all of those examples. I'm wondering if you might be able to debug this when you have time. I'll debug the SB Docs side separately, if needed, after this PR is merged. Merci beaucoup! 🙇

Additionally there are a lot of other errors, e.g. for loading MDX. I'm not sure the best way to deal with these.

p => p.key.name === 'parameters' && p.value.type === 'ObjectExpression'
);

splicedSource =
Copy link
Member Author

Choose a reason for hiding this comment

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

@libetl I think the error is here when there are no parameters attached to the default export

@vercel vercel bot temporarily deployed to staging June 24, 2019 16:27 Inactive
@shilman
Copy link
Member Author

shilman commented Jun 24, 2019

@libetl I think there's a bug when using the module format when there are no parameters on the default export. I tested this in the release/docs-technical-preview branch and it works on cra-mdx (a test project) but not in official-storybook (I tried commenting out various load lines, but all broke in the same place). Would you mind taking a look at this please?

@libetl
Copy link
Member

libetl commented Jun 24, 2019

fixed. But there seems to be something wrong after the load. all the stories are displaying errors. Can you help me Michael ?

@shilman
Copy link
Member Author

shilman commented Jun 25, 2019

@libetl Your fix worked but there is still a problem with some of the stories. In PropsTable.stories.tsx:

const { row: stringRow } = rowStories.string().props;
const { row: numberRow } = rowStories.number().props;
export const normal = () => <PropsTable rows={[stringRow, numberRow]} />;

When the source loader modifies the stories, the generated code assumes that the story functions are being called with context, and these are not.

Proposed solution:

  1. I will modify the story format per Addon-docs: Support non-story exports in module story format #7071 and modify these stories to export the data directly
  2. Later we could modify the source-loader to use injected story parameters instead of the event

@libetl
Copy link
Member

libetl commented Jun 25, 2019

Ok, tell me if I can help in any way

@shilman
Copy link
Member Author

shilman commented Jun 25, 2019

@libetl I was able to fix the problem as mentioned above.

There are two remaining problems, and I'm going to merge this PR and I've filed a separate issue to track them: #7192

@shilman shilman merged commit 6668d32 into next Jun 25, 2019
@shilman shilman deleted the 7101-source-loader branch June 25, 2019 10:55
@@ -77,6 +78,7 @@ module.exports = {
'./lib/core/src/server',
'./lib/node-logger',
'./lib/codemod',
'./lib/source-loader/src',
Copy link
Member

Choose a reason for hiding this comment

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

This directory includes client-side files like preview.js, we shouldn't apply node target to them

Copy link
Member

Choose a reason for hiding this comment

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

I'll extract the server part there

Copy link
Member

Choose a reason for hiding this comment

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

@@ -0,0 +1,6 @@
import { transform } from './build';

export { STORY_EVENT_ID } from './events';
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 is that export used anywhere? I'm going to remove it from index

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

5 participants