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

Reflect the new peer dependencies in docs and CLI templates #2714

Merged
merged 3 commits into from
Jan 11, 2018

Conversation

Hypnosphi
Copy link
Member

Issue: As someone has pointed out, both quick and slow start guides don't work currently, because they're missing the newly added peerDependencies

@Hypnosphi Hypnosphi added cli patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jan 10, 2018
@Hypnosphi Hypnosphi requested a review from a team January 10, 2018 23:19

Make sure that you have `react` and `react-dom` in your dependencies as well because we list these as a peerDependency:
Make sure that you have `react`, `react-dom`, and `babel-core` in your dependencies as well because we list these as a peerDependency:
Copy link
Member

Choose a reason for hiding this comment

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

I think this might read better as:

Make sure that you have `react`, `react-dom` in your dependencies and and `babel-core` in your devDependencies, because we list these as a peerDependency:

Copy link
Member Author

Choose a reason for hiding this comment

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

If one has babel-core in their dependencies, it will work as well

Copy link
Member

Choose a reason for hiding this comment

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

yes, but it reads as if babel-core needs to be an actual dependency, I guess I am too literal with the word dependencies 🤷‍♂️

@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

Merging #2714 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2714   +/-   ##
=======================================
  Coverage   34.35%   34.35%           
=======================================
  Files         390      390           
  Lines        8772     8772           
  Branches      909      907    -2     
=======================================
  Hits         3014     3014           
- Misses       5135     5164   +29     
+ Partials      623      594   -29
Impacted Files Coverage Δ
addons/viewport/src/components/RotateViewport.js 22.72% <0%> (ø) ⬆️
addons/info/src/components/types/Enum.js 0% <0%> (ø) ⬆️
lib/components/src/table/table.js 83.33% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 67.64% <0%> (ø) ⬆️
...tories_panel/stories_tree/tree_decorators_utils.js 45.23% <0%> (ø) ⬆️
addons/viewport/src/components/viewportInfo.js 36.36% <0%> (ø) ⬆️
...t-native/src/preview/components/StoryView/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/layout.js 12.5% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 33.96% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
... and 51 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...09095f6. Read the comment docs.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM, though the snapshot test is failing due to dep ordering. (I tried updating it it on my local machine, but the snapshot order looks correct to me?!!)

@Hypnosphi
Copy link
Member Author

Looks like there's some kind of race condition, will fix

@Hypnosphi Hypnosphi merged commit e0125e7 into master Jan 11, 2018
@Hypnosphi Hypnosphi deleted the cli-babel-core branch January 11, 2018 11:09
shilman pushed a commit that referenced this pull request Jan 13, 2018
Reflect the new peer dependencies in docs and CLI templates
@Hypnosphi Hypnosphi added patch:done Patch/release PRs already cherry-picked to main/release branch and removed patch:done Patch/release PRs already cherry-picked to main/release branch labels Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants