-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Upgrade React to 16.8.4: Welcome React Hooks #14400
Conversation
05617b8
to
dd24509
Compare
* Upgrade React to 16.8.4 * Update package-lock.json file * Expose React Hooks
dd24509
to
630e7bb
Compare
package-lock.json
Outdated
"react": "^16.8.4", | ||
"react-dom": "^16.8.4" | ||
}, | ||
"dependencies": { |
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 part should be manually removed. The same versions of packages are now in the root of the lock file.
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.
Why do we need to do these manual tweaks? It seems to work as is?
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 I think it's because they're considered "dev" dependencies at the root level, this means npm
marks them as "dev": true,
at the root level but also add them here as normal dependencies. I think we should keep them as is.
"react-test-renderer": "16.6.3", | ||
"react": "16.8.4", | ||
"react-dom": "16.8.4", | ||
"react-test-renderer": "16.8.4", |
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's surprising that react-test-renderer
wasn't listed before 🤷♂️
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.
react wasn't listed before.
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 :)
I added cd61281 which performs some cleanup to ensure we use only one version of React dependencies. |
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.
Tested locally, everything works great. You can 🚢 if you are fine with the lock file changes I applied.
|
||
[src/index.js#L1-L1](src/index.js#L1-L1) | ||
|
||
Make React Hooks available |
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.
These aren't the most helpful documentation.
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.
Mmm Right I didn't notice, we might want to just link to the React Docs here. how can we improve this, I'm not yet familiar with this new doc generator @nosolosw
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 can do a couple of options:
-
Change this comment to Refer to React docs https://reactjs.org/docs/hooks-reference.html. That'd the quick/easy way.
-
Export every item independently as we did with the others. In the JSDoc comment, we can refer to the specific React hook reference like
@see https://reactjs.org/docs/hooks-reference.html#usecallback
.
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'm preparing a PR for the second option.
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.
Awesome Thank you :)
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.
#14425 could be the minimum viable docs.
…/fix-develop Merge up until just before "Upgrade React to 16.8.4: Welcome React Hooks (#14400)"
At the risk of stating the obvious, it should be noted that in bringing |
Second attempt to upgrade React. Now that react-native 0.59 stable is out, we can move forward with the web. The mobile team can update when they sync master to develop.