-
Notifications
You must be signed in to change notification settings - Fork 27k
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
add react and react dom as peer #1024
Conversation
package.json
Outdated
@@ -112,6 +110,10 @@ | |||
"webpack-stream": "3.2.0", | |||
"cross-env": "^3.1.4" | |||
}, | |||
"peerDependencies": { | |||
"react": "^0.14.0 || ^15.0.0", | |||
"react-dom": "^0.14.0 || ^15.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't support 0.14.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol. We were reviewing at the same time 😅 Beat me to it.
I think we should add |
package.json
Outdated
@@ -112,6 +110,10 @@ | |||
"webpack-stream": "3.2.0", | |||
"cross-env": "^3.1.4" | |||
}, | |||
"peerDependencies": { | |||
"react": "^0.14.0 || ^15.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we require 15.4.2
I guess we shouldn't still support 0.14.0
then? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so ^15.4.2
would be better?
Additionally, I believe we have to remove settings of |
.gitignore
Outdated
@@ -13,3 +13,6 @@ npm-debug.log | |||
# coverage | |||
.nyc_output | |||
coverage | |||
|
|||
# editors | |||
.idea/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be managed by using https://help.github.com/articles/ignoring-files/#create-a-global-gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be, but most people don't use this as standard .gitignores generated by github already include:
# Editors
.idea/*
*.iml
*.sublime-*
by extending the .gitignore you can avoid accidental commits, but I also can remove it again if you want me to. Now when thinking about it we should probably add all the editorfiles to .gitignore and not only webstorms .idea/ files :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point 😄 Lets keep it 👍
"preact-compat": "^3.9.4" | ||
"preact-compat": "^3.9.4", | ||
"react": "^15.4.2", | ||
"react-dom": "^15.4.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need react even if we use preact
or inferno
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need it, but shrinkwrap will error out with some awkward errors due to missing deps - see the issue posted above. I can remove it again, if you want me to.
@nkzawa not entirely sure what you mean by:
|
We use https://github.com/zeit/next.js/blob/master/server/build/babel/preset.js#L23 We have to remove them. |
- tackles vercel#997 - add ./idea to gitignore for webstorm users - update all the examples
962f67a
to
362fbb0
Compare
@nkzawa okay i removed it :) please ping me if there's anything left i have to do. |
"react-dom": "15.4.2" | ||
}, | ||
"peerDependencies": { | ||
"react": "^15.4.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkzawa is this a strict requirement to have this version.
May be support a range like 15.x.x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ^
means that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. But that's after 15.4.2
.
Do we need to support some older releases in 15.x.x ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, I thought they are equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the reason to support an older version of react 15? Seems totally fine to start asking for ^15.4.2
, since we included it out of the box for all this time already 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timneutkens good point. If someone asked for a older one in 15.x.x. Let's consider to changing this.
This looks great to me. |
fixes #997
I added react and react dom to all the examples(even preact) to avoid bugs with shrinkwrap in npm v3 npm/npm#12909