Refactor framework-independent core library #2241

shilman · 2017-11-06T11:20:03Z

What I did

Work in progress: first proof of concept for discussion

create @storybook/core library
move most common code into library
add some per-framework configuration for decorators

Still to-do:

How to test

yarn bootstrap --core

cd examples/cra-kitchen-sink
yarn storybook
cd ../vue-kitchen-sink
yarn storybook
cd ../angular-cli
yarn storybook
cd ../..
yarn test

Is this testable with jest or storyshots?

Does this need a new example in the kitchen sink apps?

Does this need an update to the documentation?

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

Questionable changes: - ability to pass a story decorator function - pass clearDecorators to ConfigAPI React/Vue/Angular working (apparently). RN still broken.

danielduan · 2017-11-06T15:53:51Z

Is the plan to move most of the server stuff over as well or should that be a separate package?

ndelangen · 2017-11-06T16:38:57Z

app/react/src/client/preview/init.js

@@ -1,11 +1,11 @@
 import keyEvents from '@storybook/ui/dist/libs/key_events';
-import { selectStory } from './actions';
+import { Actions } from '@storybook/core/client';


Would exporting & importing all actions individually be better here?

I chose to bundle them to keep the namespace smaller. Open to counter-arguments.

ndelangen · 2017-11-06T16:40:12Z

lib/core/README.md

@@ -0,0 +1,13 @@
+# Storybook for React
+
+[![Greenkeeper badge](https://badges.greenkeeper.io/storybooks/storybook.svg)](https://greenkeeper.io/)


outdated readme

Fixed, though somehow it's not showing up here. Still need to write the actual contents and it's in the TODO list at top of PR.

tmeasday · 2017-11-08T05:51:00Z

Some ideas on how we could improve this further:

Can we make ClientApi/ConfigApi self-bind its methods (ala React ES6 class components), so we don't have to repeat this stuff: https://github.com/storybooks/storybook/blob/shilman/refactor-core/app/react/src/client/preview/index.js#L34-L38
Can we factor out init.js? It seems identical between the 3 web-based frameworks
Can we factor out common parts of index.js from the 3 web frameworks as well? I feel like there are bugs involved in the query parsing stuff that would be good to fix just once ;)

- resolve conflicts - fix RN story_store (make it an EventEmitter)

codecov · 2017-11-08T06:40:04Z

Codecov Report

Merging #2241 into release/3.3 will decrease coverage by 0.48%.
The diff coverage is 9.83%.

@@               Coverage Diff               @@
##           release/3.3    #2241      +/-   ##
===============================================
- Coverage        22.66%   22.18%   -0.49%     
===============================================
  Files              346      338       -8     
  Lines             6767     6587     -180     
  Branches           887      856      -31     
===============================================
- Hits              1534     1461      -73     
+ Misses            4587     4483     -104     
+ Partials           646      643       -3

Impacted Files	Coverage Δ
lib/core/src/client/preview/reducer.js	`0% <ø> (ø)`
...react-native/src/manager/components/PreviewHelp.js	`0% <ø> (ø)`	⬆️
lib/components/src/highlight_button.js	`0% <ø> (ø)`	⬆️
lib/core/src/client/preview/actions.js	`0% <ø> (ø)`
app/react-native/src/preview/index.js	`0% <0%> (ø)`	⬆️
app/angular/src/client/preview/init.js	`0% <0%> (ø)`	⬆️
lib/core/src/client/preview/config_api.js	`0% <0%> (ø)`
lib/core/src/client/index.js	`0% <0%> (ø)`
lib/core/src/client/preview/index.js	`0% <0%> (ø)`
app/react/src/client/preview/init.js	`0% <0%> (ø)`	⬆️
... and 53 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 79fb667...e8f76ce. Read the comment docs.

shilman · 2017-11-08T06:47:30Z

@tmeasday Updated the binding per your suggestion.

I didn't factor out init.js in the first pass because it depends on the UI library and I didn't want to introduce that dependency into core. But agree that it's dupe code and should be factored out. Same with the query-string code. Open to suggestions on the API for this if you have ideas! Otherwise, will take a crack at this in the next day or two.

Thanks!

tmeasday · 2017-11-08T10:36:30Z

I was thinking just something like:

import { init } from '@storybook/core/web';
init(); // <- we could pass args if ever needed to

// or even just
import '@storybook/core/init-web';

Hypnosphi · 2017-11-08T10:37:30Z

@tmeasday do you mean import '@storybook/core/init-web'?

tmeasday · 2017-11-08T10:38:20Z

@tmeasday do you mean import '@storybook/core/init-web'?

@Hypnosphi too fast off the mark there ;) Gotta give me time to fix my mistakes

shilman · 2017-11-09T04:41:53Z

@danielduan long-term will refactor everything, including preview, manager, and server. starting with preview since that's where most of the action is.

- move keyboard handling to the UI library, rather than introducing new deps - keep QS handling in each framework (for now)

codecov-io · 2017-11-09T05:21:10Z

Codecov Report

Merging #2241 into master will decrease coverage by 13.62%.
The diff coverage is 6.74%.

@@             Coverage Diff             @@
##           master    #2241       +/-   ##
===========================================
- Coverage   32.69%   19.07%   -13.63%     
===========================================
  Files         398      324       -74     
  Lines        8837     7666     -1171     
  Branches      967      820      -147     
===========================================
- Hits         2889     1462     -1427     
- Misses       5271     5567      +296     
+ Partials      677      637       -40

Impacted Files	Coverage Δ
app/react/src/client/preview/render.js	`0% <ø> (ø)`	⬆️
lib/core/src/client/preview/reducer.js	`0% <ø> (ø)`
lib/core/src/client/preview/actions.js	`0% <ø> (ø)`
lib/components/src/highlight_button.js	`0% <ø> (ø)`	⬆️
lib/core/src/client/preview/index.js	`0% <0%> (ø)`
lib/core/src/client/preview/story_store.js	`0% <0%> (ø)`
lib/core/src/client/index.js	`0% <0%> (ø)`
app/react-native/src/preview/index.js	`0% <0%> (ø)`	⬆️
lib/core/src/client/preview/config_api.js	`0% <0%> (ø)`
lib/ui/src/libs/key_events.js	`18.86% <0%> (-21.14%)`	⬇️
... and 184 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 a9d7440...9c3ec7f. Read the comment docs.

shilman · 2017-11-09T05:25:56Z

@tmeasday refactored init per our slack chat. will pull out the query string handling when i try to deal with the manager code. short-term was able to remove a few fields from the context object.

tmeasday

👏

mAAdhaTTah · 2017-11-13T03:04:19Z

Would this make it feasible for a framework author to integrate with Storybook? Is there a plan to make that possible?

tmeasday · 2017-12-15T06:25:10Z

@shilman -- what's up with this PR? I was looking at making some core changes but it feels weird to touch code before this PR has been merged. Are we waiting to merge it post 3.3?

ndelangen · 2017-12-16T10:17:26Z

Yes, we are @tmeasday, maybe you can base your work on this PR, or even on this branch?

For #2587

…factor Use store revisions to ensure that stories re-render on HMR.

tmeasday · 2018-01-03T22:02:29Z

Great to get this merged! Shall we open an issue to discuss the remaining refactoring candidates?

ndelangen · 2018-01-03T22:05:03Z

Yes sounds good!

tmeasday · 2018-01-03T22:18:01Z

=> #2636

shilman added 2 commits November 6, 2017 00:52

Refactor StoryStore into @storybook/core

9eca833

Refactor client / config / redux API into core

4573883

Questionable changes: - ability to pass a story decorator function - pass clearDecorators to ConfigAPI React/Vue/Angular working (apparently). RN still broken.

shilman requested review from tmeasday and a team November 6, 2017 11:21

danielduan approved these changes Nov 6, 2017

View reviewed changes

ndelangen self-requested a review November 6, 2017 16:35

ndelangen added maintenance User-facing maintenance tasks angular react react-native vue labels Nov 6, 2017

ndelangen assigned shilman Nov 6, 2017

ndelangen reviewed Nov 6, 2017

View reviewed changes

Merge updates from release/3.3

f938dd4

- resolve conflicts - fix RN story_store (make it an EventEmitter)

Self-binding core client/config API

e8f76ce

Remove framework init.js

dd4e813

- move keyboard handling to the UI library, rather than introducing new deps - keep QS handling in each framework (for now)

tmeasday approved these changes Nov 9, 2017

View reviewed changes

danielduan added the in progress label Nov 9, 2017

ndelangen added 2 commits November 13, 2017 07:50

Merge branch 'release/3.3' into shilman/refactor-core

4039493

Merge branch 'release/3.3' into shilman/refactor-core

0fefa98

mAAdhaTTah mentioned this pull request Nov 20, 2017

Refactor Framework-independent storybook code #2240

Closed

2 tasks

ndelangen changed the base branch from release/3.3 to master December 23, 2017 23:46

tmeasday mentioned this pull request Dec 28, 2017

Use store revisions to ensure that stories re-render on HMR. #2588

Merged

Use store revisions to ensure that stories re-render on HMR.

10ed61d

For #2587

tmeasday mentioned this pull request Dec 29, 2017

Use store revisions to ensure that stories re-render on HMR. #2605

Merged

Merge pull request #2605 from storybooks/tmeasday/add-revisions-to-re…

9c3ec7f

…factor Use store revisions to ensure that stories re-render on HMR.

tmeasday mentioned this pull request Jan 3, 2018

Merge master into shilman/refactor-core #2628

Merged

ndelangen merged commit 9c3ec7f into master Jan 3, 2018

tmeasday deleted the shilman/refactor-core branch January 3, 2018 22:02

tmeasday mentioned this pull request Jan 3, 2018

Continue refactoring out common code from app layers #2636

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor framework-independent core library #2241

Refactor framework-independent core library #2241

shilman commented Nov 6, 2017 •

edited

Loading

danielduan commented Nov 6, 2017

ndelangen Nov 6, 2017

shilman Nov 9, 2017

ndelangen Nov 6, 2017

shilman Nov 9, 2017

tmeasday commented Nov 8, 2017 •

edited by shilman

Loading

codecov bot commented Nov 8, 2017 •

edited

Loading

shilman commented Nov 8, 2017

tmeasday commented Nov 8, 2017 •

edited

Loading

Hypnosphi commented Nov 8, 2017

tmeasday commented Nov 8, 2017

shilman commented Nov 9, 2017

codecov-io commented Nov 9, 2017 •

edited by codecov bot

Loading

shilman commented Nov 9, 2017

tmeasday left a comment

mAAdhaTTah commented Nov 13, 2017 •

edited

Loading

tmeasday commented Dec 15, 2017

ndelangen commented Dec 16, 2017

tmeasday commented Jan 3, 2018

ndelangen commented Jan 3, 2018

tmeasday commented Jan 3, 2018

		@@ -0,0 +1,13 @@
		# Storybook for React

		[![Greenkeeper badge](https://badges.greenkeeper.io/storybooks/storybook.svg)](https://greenkeeper.io/)

Refactor framework-independent core library #2241

Refactor framework-independent core library #2241

Conversation

shilman commented Nov 6, 2017 • edited Loading

What I did

How to test

danielduan commented Nov 6, 2017

ndelangen Nov 6, 2017

Choose a reason for hiding this comment

shilman Nov 9, 2017

Choose a reason for hiding this comment

ndelangen Nov 6, 2017

Choose a reason for hiding this comment

shilman Nov 9, 2017

Choose a reason for hiding this comment

tmeasday commented Nov 8, 2017 • edited by shilman Loading

codecov bot commented Nov 8, 2017 • edited Loading

Codecov Report

shilman commented Nov 8, 2017

tmeasday commented Nov 8, 2017 • edited Loading

Hypnosphi commented Nov 8, 2017

tmeasday commented Nov 8, 2017

shilman commented Nov 9, 2017

codecov-io commented Nov 9, 2017 • edited by codecov bot Loading

Codecov Report

shilman commented Nov 9, 2017

tmeasday left a comment

Choose a reason for hiding this comment

mAAdhaTTah commented Nov 13, 2017 • edited Loading

tmeasday commented Dec 15, 2017

ndelangen commented Dec 16, 2017

tmeasday commented Jan 3, 2018

ndelangen commented Jan 3, 2018

tmeasday commented Jan 3, 2018

shilman commented Nov 6, 2017 •

edited

Loading

tmeasday commented Nov 8, 2017 •

edited by shilman

Loading

codecov bot commented Nov 8, 2017 •

edited

Loading

tmeasday commented Nov 8, 2017 •

edited

Loading

codecov-io commented Nov 9, 2017 •

edited by codecov bot

Loading

mAAdhaTTah commented Nov 13, 2017 •

edited

Loading