Skip to content

add react and react dom as peer#1024

Merged
arunoda merged 1 commit intovercel:masterfrom
sakulstra:feature/react-as-peer-#997
Feb 8, 2017
Merged

add react and react dom as peer#1024
arunoda merged 1 commit intovercel:masterfrom
sakulstra:feature/react-as-peer-#997

Conversation

@sakulstra
Copy link
Copy Markdown
Contributor

@sakulstra sakulstra commented Feb 7, 2017

fixes #997

  • add ./idea to gitignore for webstorm users
  • update all the examples

I added react and react dom to all the examples(even preact) to avoid bugs with shrinkwrap in npm v3 npm/npm#12909

package.json Outdated
},
"peerDependencies": {
"react": "^0.14.0 || ^15.0.0",
"react-dom": "^0.14.0 || ^15.0.0"
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

@timneutkens timneutkens Feb 7, 2017

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.

@nkzawa
Copy link
Copy Markdown
Contributor

nkzawa commented Feb 7, 2017

I think we should add react and react-dom to devDependencies.

package.json Outdated
"cross-env": "^3.1.4"
},
"peerDependencies": {
"react": "^0.14.0 || ^15.0.0",
Copy link
Copy Markdown
Member

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? 🤔

Copy link
Copy Markdown
Contributor Author

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?

@nkzawa
Copy link
Copy Markdown
Contributor

nkzawa commented Feb 7, 2017

Additionally, I believe we have to remove settings of babel-plugin-module-resolver from babel.

.gitignore Outdated
coverage

# editors
.idea/ No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

@sakulstra sakulstra Feb 7, 2017

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 :)

Copy link
Copy Markdown
Member

@timneutkens timneutkens Feb 7, 2017

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"
Copy link
Copy Markdown
Contributor

@nkzawa nkzawa Feb 7, 2017

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 ?

Copy link
Copy Markdown
Contributor Author

@sakulstra sakulstra Feb 7, 2017

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.

@sakulstra
Copy link
Copy Markdown
Contributor Author

@nkzawa not entirely sure what you mean by:

Additionally, I believe we have to remove settings of babel-plugin-module-resolver from babel.

@nkzawa
Copy link
Copy Markdown
Contributor

nkzawa commented Feb 7, 2017

We use babel-plugin-module-resolver to be able to use React on your apps like this.

https://github.com/zeit/next.js/blob/master/server/build/babel/preset.js#L23
https://github.com/zeit/next.js/blob/master/server/build/webpack.js#L178

We have to remove them.

- tackles vercel#997
- add ./idea to gitignore for webstorm users
- update all the examples
@sakulstra sakulstra force-pushed the feature/react-as-peer-#997 branch from 962f67a to 362fbb0 Compare February 7, 2017 11:53
@sakulstra
Copy link
Copy Markdown
Contributor Author

@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",
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think ^ means that ?

Copy link
Copy Markdown
Contributor

@arunoda arunoda Feb 7, 2017

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 ?

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member

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 😄

Copy link
Copy Markdown
Contributor

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.

@arunoda
Copy link
Copy Markdown
Contributor

arunoda commented Feb 8, 2017

This looks great to me.
Thanks @sakulstra

@arunoda arunoda merged commit 4a73ccb into vercel:master Feb 8, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants