Source-loader: Overhaul to remove decorators, support user-configurable source #9547

atanasster · 2020-01-20T06:18:49Z

Issue:

What I did

refactor source-loader to reduce code changes to minimum (only the story source and the id map is kept)
removed decorators of stories that used to 'emit' source code, now only the parameters: { storySource: { source, locationsMap } } are used
refactor addons/StorySource to use story parameters instead of replying on messages from source-loader code
rewrite addons/StorySource in typescript

How to test

Hopefully all should work as before, just less code generated and less possible sources of issues.
I am not that familiar with older formats - if you have stories using storiesOf or any third-party addons using source-loader would be great help.

vercel · 2020-01-20T06:18:53Z

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/9i4evbn98
✅ Preview: https://monorepo-git-fork-atanasster-source-loader-refactor.storybook.now.sh

shilman

THIS IS AWESOME!!!

I think we can make a few more tweaks (see comments), but it's a HUGE improvement 😍

addons/docs/src/blocks/Source.tsx

addons/storysource/package.json

addons/storysource/src/StoryPanel.tsx

shilman · 2020-01-21T04:05:08Z

addons/storysource/src/StoryPanel.tsx

+}
+export const StoryPanel: React.FC<StoryPanelProps> = ({ api }) => {
+  const [state, setState] = React.useState<SourceParams & { currentLocation?: SourceBlock }>({
+    source: 'loading source...',


is there any way to detect when the source-loader failed and show an error message?

maybe if no source code is found at all in the storySourc parameter?

shilman · 2020-01-21T04:06:45Z

addons/storysource/src/StoryPanel.tsx

+  kind?: string;
+  parameters?: {
+    storySource?: SourceParams;
+    mdxSource?: string;


while we're cleaning this up, maybe we can get rid of mdxSource? i think now you support an empty locationsMap, so we can put the MDX source in storySource.source and it's a lot cleaner.

it might break some code, no?
I saw examples in the code like:
two.story.parameters = { mdxSource: '<Button>Two</Button>' };

addons/storysource/src/preview.ts

examples/html-kitchen-sink/stories/source-loader.stories.js

lib/source-loader/src/server/build.js

atanasster · 2020-01-21T15:58:40Z

@shilman - checked in the updated code, without the removal of mdxSource.

atanasster · 2020-01-21T16:03:45Z

For mdxSource we can:

remove mdxSource completely and mark it as a breaking change for 6.0 for both Preview and parameters.
keep mdxSource as a prop to Preview only, remove it from parameters and still mark it as a breaking change.
keep mdxSource backwards compatible, but change mdx-compiler to output to storySource. All the code in Source, Preview will still need to check for both mdxSource and storySource parameters.

shilman · 2020-01-21T16:14:28Z

@atanasster Removing mdxSource is all about cleaning things up (assuming it does clean things up--if it doesn't, let's discuss!), so let's remove it completely if we make the change. Docs is quite new and hopefully people will understand that the API is still in flux, esp. in a major release. Furthermore mdxSource is an undocumented advanced usage.

atanasster · 2020-01-21T16:16:11Z

@shilman - yes, makes sense.

Would you like to first merge this PR, its gotten quite large already and remove mdxSource in another PR?

shilman · 2020-01-21T16:19:32Z

@atanasster sounds good. will do!

ndelangen · 2020-01-28T01:22:31Z

Amazing job on this! Didn't mean to close the PR at all. Really appreciate the source-loader getting an improvement like this!

atanasster · 2020-01-28T03:30:15Z

Cool, its not an improvement, its a re-design and a rewrite and i would really appreciate this getting merged in the alphas to get some testing. Its a lot of work, including the full typescript rewrite of the source addon.

atanasster · 2020-01-28T03:34:12Z

And just in case its missed - the source-loader was doing ungodly things manipulating the source code, probably useful at the time, but now affecting both for performance and usability and a rewrite was a bit overdue.

shilman · 2020-01-28T07:32:16Z

@atanasster Thanks for your patience on this. I'm merging this with the understanding that this is the start of an iterative process, but that merging & releasing will get the ball rolling.

We need to figure out a good process for experimenting with new code. You suggested an "experimental" branch, but it's already cumbersome enough to patch changes back and forth between next and master, and in the past when we had three branches it's been untenable for me as the primary person responsible for keeping track. I think, instead, that I'll more aggressively merge stuff into next provided that there is some understanding about follow-up work.

In this case, the follow-up work probably includes:

Getting rid of mdxSource if we can
Addressing the DeepScan warnings
Testing with users and iterating based on the issues that come up (as with any change we introduce)
Updating the documentation to reflect the changes

There's probably more, but I trust that we can figure it all out over the coming weeks.

ndelangen · 2020-01-28T09:07:28Z

🎉

atanasster · 2020-01-28T16:32:09Z

Thanks a bunch @shilman as we discussed this was blocking me from testing the default ‘knobs-2’ passing props as the first parameter to storyFn

I am a happy camper again, sorry for venting my frustration.

atanasster added 4 commits January 19, 2020 23:35

initial check0in

554b379

clean up further

a38d2a4

re-generate test snapshots

a810ff4

removed unused code

d77bbcf

atanasster added ci: do not merge addon: storysource source-loader labels Jan 20, 2020

atanasster requested review from igor-dv, ndelangen and tmeasday as code owners January 20, 2020 06:18

vercel bot deployed to Preview January 20, 2020 06:18 View deployment

atanasster removed the ci: do not merge label Jan 20, 2020

atanasster mentioned this pull request Jan 20, 2020

Displaying imported HTML in Storysource #8297

Closed

withStorySource deprecation

95c4c7d

vercel bot deployed to Preview January 20, 2020 18:18 View deployment

fix parameters, when locationsMap is null

2937617

vercel bot deployed to Preview January 20, 2020 18:44 View deployment

shilman requested changes Jan 21, 2020

View reviewed changes

code changes according to shilman comments

e3bf436

vercel bot deployed to Preview January 21, 2020 15:56 View deployment

Merge branch 'next' into pr/9547

11d057c

atanasster requested a review from thomasbertet as a code owner January 21, 2020 16:29

vercel bot had a problem deploying to Preview January 21, 2020 16:29 Failure

shilman added the maintenance User-facing maintenance tasks label Jan 21, 2020

atanasster mentioned this pull request Jan 22, 2020

Show more source files in storysource #3566

Open

ndelangen closed this Jan 28, 2020

ndelangen reopened this Jan 28, 2020

shilman changed the title ~~Source loader refactor~~ Source-loader: Overhaul for user-configurable source, no decorators Jan 28, 2020

shilman added 2 commits January 28, 2020 15:16

Merge branch 'next' into pr/9547

8a49725

Create source-loader.stories.storyshot

d0096dc

shilman added this to the 6.0.0 milestone Jan 28, 2020

shilman changed the title ~~Source-loader: Overhaul for user-configurable source, no decorators~~ Source-loader: Overhaul to remove decorators, support user-configurable source Jan 28, 2020

shilman merged commit 2bee5dd into storybookjs:next Jan 28, 2020

shilman deleted the source-loader-refactor branch January 28, 2020 08:47

This was referenced Mar 9, 2020

MDX: Compile to improved source-loader format #10084

Merged

Storysource addon does not work with CRA + MDX #9930

Closed

dependencies bot mentioned this pull request Jan 15, 2021

Update 2 dependencies from npm #2290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source-loader: Overhaul to remove decorators, support user-configurable source #9547

Source-loader: Overhaul to remove decorators, support user-configurable source #9547

atanasster commented Jan 20, 2020 •

edited

Loading

vercel bot commented Jan 20, 2020 •

edited

Loading

shilman left a comment

shilman Jan 21, 2020

atanasster Jan 21, 2020

shilman Jan 21, 2020

atanasster Jan 21, 2020

atanasster commented Jan 21, 2020

atanasster commented Jan 21, 2020

shilman commented Jan 21, 2020 •

edited

Loading

atanasster commented Jan 21, 2020

shilman commented Jan 21, 2020

ndelangen commented Jan 28, 2020

atanasster commented Jan 28, 2020

atanasster commented Jan 28, 2020

shilman commented Jan 28, 2020 •

edited

Loading

ndelangen commented Jan 28, 2020

atanasster commented Jan 28, 2020 •

edited

Loading

Source-loader: Overhaul to remove decorators, support user-configurable source #9547

Source-loader: Overhaul to remove decorators, support user-configurable source #9547

Conversation

atanasster commented Jan 20, 2020 • edited Loading

What I did

How to test

vercel bot commented Jan 20, 2020 • edited Loading

shilman left a comment

Choose a reason for hiding this comment

shilman Jan 21, 2020

Choose a reason for hiding this comment

atanasster Jan 21, 2020

Choose a reason for hiding this comment

shilman Jan 21, 2020

Choose a reason for hiding this comment

atanasster Jan 21, 2020

Choose a reason for hiding this comment

atanasster commented Jan 21, 2020

atanasster commented Jan 21, 2020

shilman commented Jan 21, 2020 • edited Loading

atanasster commented Jan 21, 2020

shilman commented Jan 21, 2020

ndelangen commented Jan 28, 2020

atanasster commented Jan 28, 2020

atanasster commented Jan 28, 2020

shilman commented Jan 28, 2020 • edited Loading

ndelangen commented Jan 28, 2020

atanasster commented Jan 28, 2020 • edited Loading

atanasster commented Jan 20, 2020 •

edited

Loading

vercel bot commented Jan 20, 2020 •

edited

Loading

shilman commented Jan 21, 2020 •

edited

Loading

shilman commented Jan 28, 2020 •

edited

Loading

atanasster commented Jan 28, 2020 •

edited

Loading