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

Angular and Vue storyshots #2564

Merged
merged 75 commits into from
Jan 15, 2018
Merged

Angular and Vue storyshots #2564

merged 75 commits into from
Jan 15, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Dec 26, 2017

Issue: Adding storyshots support for angular

What I did

  1. I have a BDay - this is my cake =) 🍰
  2. I've refactored storyshots to make it easier to integrate new frameworks
  3. There are these PRs (add support for storyshots to save files according to different patterns #2517, Add ability to use image snapshots to addon-storyshots #2413) that should happen before
  4. I have concerns about adding every new framework to peer dep... Should people install all these angular/polymer/aurelia packages?
  5. There is a lot of copy-paste - TBD in a sperate PR - extract common angular logic to some kind of @storybook/angular-core
  6. I don't know how to make props to be visible in snapshots =(
  7. One storyshot is failing. We need to refactor more things to be able to run framework-specific preparations with beforeEach.

How to test

For running only angular sthoryshots:

node node_modules\jest\bin\jest.js angularshots

For running only vue sthoryshots:

node node_modules\jest\bin\jest.js vueshots

@storybookjs storybookjs deleted a comment from codecov bot Jan 1, 2018
@codecov
Copy link

codecov bot commented Jan 1, 2018

Codecov Report

Merging #2564 into master will increase coverage by 1.99%.
The diff coverage is 74.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2564      +/-   ##
==========================================
+ Coverage   34.35%   36.35%   +1.99%     
==========================================
  Files         390      401      +11     
  Lines        8772     8937     +165     
  Branches      909      911       +2     
==========================================
+ Hits         3014     3249     +235     
+ Misses       5135     5090      -45     
+ Partials      623      598      -25
Impacted Files Coverage Δ
addons/storyshots/src/react/loader.js 100% <100%> (ø)
addons/storyshots/src/react/renderTree.js 100% <100%> (ø)
addons/storyshots/src/react/renderShallowTree.js 57.14% <100%> (ø)
addons/storyshots/src/config-loader.js 100% <100%> (ø)
addons/storyshots/src/index.js 87.09% <100%> (+7.09%) ⬆️
addons/storyshots/src/vue/renderTree.js 60% <40%> (ø)
addons/storyshots/src/angular/renderTree.js 61.9% <50%> (ø)
addons/storyshots/src/rn/loader.js 53.33% <50%> (ø)
addons/storyshots/src/hasDependency.js 50% <50%> (ø)
addons/storyshots/src/angular/loader.js 73.91% <73.68%> (ø)
... and 82 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 eeb012a...9091f4a. Read the comment docs.

@igor-dv
Copy link
Member Author

igor-dv commented Jan 10, 2018

@Hypnosphi , what do you think of using this: https://github.com/Wizcorp/codependency ?

@Hypnosphi
Copy link
Member

Description looks a bit outdated (npm stopped installing peers quite a long time ago)

@Hypnosphi
Copy link
Member

Probably that's because it was last updated in 2014

@igor-dv
Copy link
Member Author

igor-dv commented Jan 10, 2018

I think I've found a solution. Now react-test-renderer is loaded only when used with react storyshots. I can remove it from deps (also jest-preset-angular). Need only update the docs.

@@ -1,4 +1,6 @@
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Member

@Hypnosphi Hypnosphi Jan 10, 2018

Choose a reason for hiding this comment

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

How are people expected to get jest-preset-angular dependency? As contrary to @angular/core, it's by no means guaranteed to be present in an angular project

@@ -1,3 +1,4 @@
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

People who are already using Storyshots were already needed to install react-test-renderer, so for them, there won't be a problem. I need to add a section in Readme about these "optional peers", that people have to install by hand.
BTW, For angular and vue these deps are needed to configure jestconfig (I've already added it to readme)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me

@@ -36,6 +38,43 @@ Usually, you might already have completed this step. If not, here are some resou

> Note: If you use React 16, you'll need to follow [these additional instructions](https://github.com/facebook/react/issues/9102#issuecomment-283873039).

### Configure Jest for React
StoryShots addon for React is dependent on [react-test-renderer](https://github.com/facebook/react/tree/master/packages/react-test-renderer), but
doesn't install it, so you need yo install it separately.
Copy link
Member

Choose a reason for hiding this comment

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

Please add a note explaining why we can't have it as a peer dep

Copy link
Member

@Hypnosphi Hypnosphi Jan 11, 2018

Choose a reason for hiding this comment

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

Also, "yo" is probably a typo =D

Copy link
Member

Choose a reason for hiding this comment

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

Please add the shell command to copypaste:

npm install --save-dev react-test-renderer

@@ -40,11 +40,19 @@ Usually, you might already have completed this step. If not, here are some resou

### Configure Jest for React
StoryShots addon for React is dependent on [react-test-renderer](https://github.com/facebook/react/tree/master/packages/react-test-renderer), but
doesn't install it, so you need yo install it separately.
doesn't install it, so you need to install it separately (read [here](#deps-issue) why).
Copy link
Member

@Hypnosphi Hypnosphi Jan 11, 2018

Choose a reason for hiding this comment

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

It's a super-nit, but I prefer when a sentence keeps making sense if you replace links with their text, that's why I don't really like links with "here" text.

Can we just make "doesn't" a link?

### <a name="deps-issue"></a>Why don't we install dependencies of each framework ?
Storyshots addon is currently supporting React, Angular and Vue. Each framework needs its own packages to be integrated with Jest. We don't want people that use only React will need to bring other dependencies that do not make sense for them.

`dependancies` - will installed an exact version of the particular dep - Storyshots can work with different versions of the same framework (let's say React v16 and React v15), that have to be compatible with a version of its plugin (react-test-renderer).
Copy link
Member

Choose a reason for hiding this comment

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

dependancies — typo

@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

Merging #2564 into master will increase coverage by 1.89%.
The diff coverage is 73.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2564      +/-   ##
==========================================
+ Coverage   33.83%   35.73%   +1.89%     
==========================================
  Files         422      433      +11     
  Lines        9265     9430     +165     
  Branches      983      987       +4     
==========================================
+ Hits         3135     3370     +235     
+ Misses       5452     5433      -19     
+ Partials      678      627      -51
Impacted Files Coverage Δ
addons/storyshots/src/test-body-image-snapshot.js 7.69% <0%> (ø) ⬆️
addons/storyshots/src/react/renderTree.js 100% <100%> (ø)
addons/storyshots/src/react/loader.js 100% <100%> (ø)
addons/storyshots/src/config-loader.js 100% <100%> (ø)
addons/storyshots/src/react/renderShallowTree.js 57.14% <100%> (ø)
addons/storyshots/src/vue/renderTree.js 60% <40%> (ø)
addons/storyshots/src/angular/renderTree.js 61.9% <50%> (ø)
addons/storyshots/src/rn/loader.js 53.33% <50%> (ø)
addons/storyshots/src/hasDependency.js 50% <50%> (ø)
addons/storyshots/src/angular/loader.js 73.91% <73.68%> (ø)
... and 80 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 48a92e1...760d3c7. Read the comment docs.

# Conflicts:
#	addons/storyshots/README.md
#	addons/storyshots/src/index.js
@igor-dv
Copy link
Member Author

igor-dv commented Jan 15, 2018

I'm merging this one since I'm tired solving conflicts. Still didn't get a feedback on the angular part, but I think it will be easier to test in beta.

@igor-dv igor-dv merged commit ebe0ca8 into master Jan 15, 2018
@igor-dv igor-dv deleted the angular-storyshots branch January 15, 2018 19:27
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.

3 participants