Fix proptypes warning for kitchen-sink #1238
Conversation
alexandrebodin
changed the title
Codecov Report
@@ Coverage Diff @@
## 1220-kitchen-sink #1238 +/- ##
==================================================
Coverage 13.74% 13.74%
==================================================
Files 207 207
Lines 4633 4633
Branches 582 513 -69
==================================================
Hits 637 637
- Misses 3494 3544 +50
+ Partials 502 452 -50
Continue to review full report at Codecov.
|
|
@tmeasday do you mind reviewing this? I think you are the primary "user" of |
| @@ -17,7 +17,7 @@ const Button = ({ children, onClick }) => | |||
| </button>; | |||
|
|
|||
| Button.propTypes = { | |||
| children: PropTypes.string.isRequired, | |||
| children: PropTypes.oneOfType([PropTypes.string, PropTypes.array]).isRequired, | |||
| @@ -118,7 +118,7 @@ export default class Welcome extends React.Component { | |||
| } | |||
|
|
|||
| Welcome.propTypes = { | |||
| showApp: PropTypes.function, | |||
There was a problem hiding this comment.
rookie mistake, whoever wrote this... Ow wait this was me.
| "@storybook/channels": "file:../../lib/channels", | ||
| "@storybook/react": "file:../../app/react", | ||
| "@storybook/ui": "file:../../lib/ui", | ||
| "@storybook/addon-actions": "^3.0.1", |
There was a problem hiding this comment.
This removes the purpose of test-cra
I at one point set it up as an alternative if bootstrapping wouldn't work.
Nowadays bootstrapping is relatively fast-ish (compared to what is was) and it's reliable (at least for me & on CI).
I think we can discontinue this example now, and stop using file dependencies. @tmeasday I'd like your thoughts on this?
There was a problem hiding this comment.
Previously there was an advantage to test-cra because it flattened dependencies (with npm<5) and thus "looked like" a real app in terms of how require/import resolved.
However, in npm@5, file:// URLs are treated like npm links (AFAICT), so that advantage is gone. It seems like we can just about get away with it with aggressive use of peerDependencies in packages and linking every storybook related package in the app (this is OK).
So I don't think there's any point in distinguishing test-cra from cra-storybook any more.
There was a problem hiding this comment.
I definitely don't think linking to versions is what we want.
There was a problem hiding this comment.
Are you OK with me just removing it then?
There was a problem hiding this comment.
I think so if you agree with the motivation for it in the first place above.
|
I have some questions so I can continue working on this :
|
|
@alexandrebodin sorry for the confusion here. i'd love your help with this stuff. Let's go with the following: And please feel free to do as much as you'd like, or we can divide it up. BTW I've converted |
|
What I meant by peer deps is e.g. if one of the addons has a direct |
Issue:
What I did
I cleaned some proptypes in addon-info and test-cra
I got every addons to work with this config on node v8 npm 4.6
Changed test-cra deps to lerna for easy experimenting / addon updating