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

WIP - add vue support #1267

Merged
merged 69 commits into from
Jul 2, 2017
Merged

WIP - add vue support #1267

merged 69 commits into from
Jul 2, 2017

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jun 12, 2017

Issue: #1262

What we did

  • Get a working webpack build (vue-loader)
  • POC of rendering a vue component inside preview via storiesOf('myComponent').add()

How to test

After checkout out the branch locally run the commands:

$ npm install
$ npm run bootstrap
$ cd examples/vue
$ npm run storybook

TODO

@ndelangen ndelangen self-assigned this Jun 12, 2017
@codecov
Copy link

codecov bot commented Jun 12, 2017

Codecov Report

Merging #1267 into release/3.2 will decrease coverage by 0.94%.
The diff coverage is 13.89%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.2    #1267      +/-   ##
===============================================
- Coverage        15.29%   14.35%   -0.95%     
===============================================
  Files              202      234      +32     
  Lines             4706     4919     +213     
  Branches           627      686      +59     
===============================================
- Hits               720      706      -14     
- Misses            3456     3624     +168     
- Partials           530      589      +59
Impacted Files Coverage Δ
lib/cli/lib/project_types.js 0% <ø> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 11.11% <ø> (ø)
app/vue/src/client/preview/render.js 0% <0%> (ø)
addons/knobs/src/index.js 0% <0%> (ø) ⬆️
...p/vue/src/server/config/defaults/webpack.config.js 0% <0%> (ø)
lib/cli/bin/generate.js 0% <0%> (ø) ⬆️
app/vue/src/server/config.js 0% <0%> (ø)
app/vue/src/server/index.html.js 0% <0%> (ø)
addons/notes/src/index.js 0% <0%> (ø) ⬆️
app/vue/src/client/preview/actions.js 0% <0%> (ø)
... and 117 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 80f7ab2...83bb874. Read the comment docs.

@ndelangen ndelangen mentioned this pull request Jun 12, 2017
"babel-core": "^6.24.1",
"babel-loader": "^7.0.0",
"babel-plugin-react-docgen": "^1.5.0",
"babel-preset-es2015": "^6.24.1",
Copy link

Choose a reason for hiding this comment

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

It's unnecessary to have es2015, es2016 here and in .babelrc (I noticed this for all the apps (react/react-native) - can switch to preset-env instead.

"es2015", "es2016" -> "env"

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's on my todolist 😅

@alexandrebodin
Copy link
Contributor

I made some of the examples work but still a few issues to fix.

  • I add to remount for every story (not sure why)
  • We need to find a way to make sure Vue.use / Vue.component calls in the stories are processed before mounting the root Vue.
    (Maybe there are Vue tricks to declare a Component globally after but could'nt find one)

Waiting for some feedback before continuing

@@ -52,7 +52,7 @@ export default function() {
// Based on this CRA feature: https://github.com/facebookincubator/create-react-app/issues/253
modules: ['node_modules'].concat(nodePaths),
alias: {
'vue$': 'vue/dist/vue.esm.js'
'vue': path.resolve(path.join(__dirname, '../../../node_modules', 'vue/dist/vue.esm.js'))
Copy link
Member Author

Choose a reason for hiding this comment

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

this needs a explanatory comment ⁉️

Copy link
Member

Choose a reason for hiding this comment

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

why do you change it? 🤔
I think 'vue$': 'vue/dist/vue.esm.js' is correct.

See the docs:
https://vuejs.org/v2/guide/installation.html#Runtime-Compiler-vs-Runtime-only

Copy link
Contributor

Choose a reason for hiding this comment

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

Not correct in our case because of webpack being in storybook/vue and not ine the example app.

Not pointing to the correct dep will have webpack load 2 times vie and thus make it impossible to ise extension or global custom components

Copy link
Contributor

Choose a reason for hiding this comment

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

@kazupon I fyou could try to run the example s storybook and see if all the stories are working :)

@@ -4,11 +4,11 @@
"private": true,
"devDependencies": {
"react-scripts": "0.9.5",
"@storybook/vue": "^3.0.0",
"@storybook/vue": "3.0.0-alpha.0",
Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen @alexandrebodin If these deps are filled in by bootstrap, can we use *?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll try to see if that works ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@shilman Ok so. Semver will not match a prerelease tag with *. Wee need to specify -tag

I will use ^3.0.0-alpha.0 to match any of the next versions until we up app/vue version to 3.x ;)

Copy link
Member

Choose a reason for hiding this comment

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

@alexandrebodin can you explain a little more? i'm wondering if this is related to my problems on the kitchen-sink project.

@ndelangen ndelangen added the vue label Jun 14, 2017
@@ -31,7 +31,8 @@ module.exports = latestVersion('@storybook/react-native').then(version => {
packageJson.devDependencies['@storybook/react-native'] = `^${version}`;

if (!packageJson.dependencies['react-dom'] && !packageJson.devDependencies['react-dom']) {
packageJson.devDependencies['react-dom'] = '^15.5.4';
const reactVersion = packageJson.dependencies.react;
Copy link
Member

Choose a reason for hiding this comment

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

how did this get in here?

@@ -15,7 +15,8 @@ module.exports = latestVersion('@storybook/react-native').then(version => {
packageJson.devDependencies['@storybook/react-native'] = `^${version}`;

if (!packageJson.dependencies['react-dom'] && !packageJson.devDependencies['react-dom']) {
packageJson.devDependencies['react-dom'] = '^15.5.4';
const reactVersion = packageJson.dependencies.react;
Copy link
Member

Choose a reason for hiding this comment

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

how did this get in here?

Copy link
Member

Choose a reason for hiding this comment

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

This was merged from master, no?

@shilman
Copy link
Member

shilman commented Jun 24, 2017

@ndelangen @alexandrebodin I took a first pass through the code and was surprised—it looks really good!

My biggest concern at this point is addons. As far as a I can tell in the code above, when a react user loads addon-notes (for example) in dev mode, they also load vue. For a single framework, this isn't the end of the world, but if we plan to roll out a bunch of new frameworks then this is going to bloat.

Furthermore, I worry about changing the addon API with this release (e.g. addonNotes) and then changing it again once we figure out how to clean up this problem and others. I think this is an excellent case study with which we can work through our addon discussion #1212

cc @tmeasday

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.

Looks great guys! Let's get a preview alpha out!

expect(storyWrapperProps.context).toEqual(testContext);
});
});
});
Copy link
Member

Choose a reason for hiding this comment

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

It's great to have some tests for knobs! Even better would be more tests that fully exercised it. I think we can do this later: #1355

@shilman
Copy link
Member

shilman commented Jun 24, 2017

@tmeasday alpha is already live AFAIK @3.2.0-alpha.0 and has been for a few days -- these guys
are on fire 🔥

@shilman
Copy link
Member

shilman commented Jun 29, 2017

@kazupon @ndelangen @alexandrebodin mind fixing the conflicts so we can merge this and I can release an alpha.1 with vue + story hierarchy?

@alexandrebodin
Copy link
Contributor

@shilman Will do in a few hours ;)

@shilman shilman merged commit cf94190 into release/3.2 Jul 2, 2017
@shilman
Copy link
Member

shilman commented Jul 2, 2017

storybook

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.

7 participants