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

Framework: update to react 16 (take three) #19723

Merged
merged 4 commits into from
Nov 29, 2017
Merged

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Nov 13, 2017

Everyone knows the third time is the charm.

to do:

  1. interpolate components: Bump to allow for React 16, fix test interpolate-components#12, Framework: Bump i18n-calypso to 1.8.4 #19658
  2. fix ssr: enable user-bootstrapping when secrets are present #20267

To Test and Verify

  1. local dev
  2. docker. note: ssr will be broken unless secrets.json is configured.
  3. there should be no reference to React < 16 within our shrinkwrap

@matticbot
Copy link
Contributor

@kwight
Copy link
Contributor

kwight commented Nov 17, 2017

Note that notifications-panel (the react16 branch aligned with this update) is now at 2.1.1.

@sirreal
Copy link
Member

sirreal commented Nov 27, 2017

Note that notifications-panel (the react16 branch aligned with this update) is now at 2.1.1.

@kwight 2.1.1 does not seem to be published yet 😬

@sirreal sirreal force-pushed the framework/react16-take3 branch from fe665b1 to beed2bb Compare November 27, 2017 15:17
@sirreal
Copy link
Member

sirreal commented Nov 27, 2017

This had become quite out of date.

Rebased, [email protected], update-deps, force push.

@sirreal sirreal force-pushed the framework/react16-take3 branch from beed2bb to 51e8ccb Compare November 27, 2017 15:54
@sirreal
Copy link
Member

sirreal commented Nov 27, 2017

Rebased again, sorry. [email protected] was causing errors. Back to 2.0.0

@kwight
Copy link
Contributor

kwight commented Nov 27, 2017

Ack, will verify 2.1.1.

@kwight
Copy link
Contributor

kwight commented Nov 27, 2017

[email protected] published 👍

@samouri
Copy link
Contributor Author

samouri commented Nov 28, 2017

I'm basing this off the ssr fix now

@samouri samouri force-pushed the framework/react16-take3 branch from d093ff6 to 66e321e Compare November 28, 2017 20:54
@samouri samouri changed the base branch from master to config/re-enable-bootstrap November 28, 2017 20:54
@samouri samouri added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 28, 2017
@samouri
Copy link
Contributor Author

samouri commented Nov 29, 2017

🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 
⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ready for testing  ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ ⭐️ 
🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I've done the following smoke testing and encountered no issues 🎉

  • local dev
  • docker. note: ssr will be broken unless secrets.json is configured.
  • there should be no reference to React < 16 within our shrinkwrap

For SSR I see /themes is correct (logged in and logged out) if config/secrets.json is included. Otherwise I see the following message and the issue pictured here.

Disabling server-side user-bootstrapping because of a missing secrets.json

@@ -106,7 +106,7 @@
"ms": "0.7.1",
"name-all-modules-plugin": "1.0.1",
"node-sass": "4.5.3",
"notifications-panel": "1.2.7",
"notifications-panel": "2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Should we get the current 2.1.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also React 16.2!

Copy link
Member

Choose a reason for hiding this comment

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

Love those <>Fragments</> 🎉

@sirreal
Copy link
Member

sirreal commented Nov 29, 2017

Running tests now produces far fewer warnings, which is nice. Now we have a new annoyance:

console.error node_modules/fbjs/lib/warning.js:33
Warning: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers. http://fb.me/react-polyfills

Relevant conversation seems to be happening here: jestjs/jest#4545. Just a warning, so nothing blocking and we can ship a fix later.

@samouri samouri changed the base branch from config/re-enable-bootstrap to master November 29, 2017 16:17
@samouri samouri force-pushed the framework/react16-take3 branch 2 times, most recently from 0bae1d6 to 0349330 Compare November 29, 2017 16:18
@samouri samouri removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2017
@samouri samouri added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2017
package.json Outdated
@@ -277,7 +277,7 @@
"prettier": "github:automattic/calypso-prettier#717e62ee",
"pretty-bytes": "4.0.2",
"react-codemod": "reactjs/react-codemod",
"react-test-renderer": "15.6.2",
"react-test-renderer": "16.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also bump to 16.2.0?

@samouri
Copy link
Contributor Author

samouri commented Nov 29, 2017

I've had to bump back down to 2.0 due to issues withesnext

@ockham
Copy link
Contributor

ockham commented Nov 29, 2017

I've had to bump back down to 2.0 due to issues withesnext

My bad. Context: #19698

To fix, we can:

@samouri
Copy link
Contributor Author

samouri commented Nov 29, 2017

To fix, we can:

Am I right in thinking that can all happen after this PR lands?

@samouri samouri added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 29, 2017
@samouri samouri force-pushed the framework/react16-take3 branch from fd8be57 to fc1dbc3 Compare November 29, 2017 17:09
@samouri
Copy link
Contributor Author

samouri commented Nov 29, 2017

after upgrading a few packages (and then redowngrading notif-panel), i'll do another smoke test and then ask for an E2E from flow-patrol

@ockham
Copy link
Contributor

ockham commented Nov 29, 2017

Am I right in thinking that can all happen after this PR?

Oh, absolutely! Sry for not having been clear 😄

@ockham
Copy link
Contributor

ockham commented Nov 29, 2017

Am I right in thinking that can all happen after this PR?

Oh, absolutely! Sry for not having been clear 😄

As pretty much anything that doesn't make Calypso go up in 🔥s at this point 😄

@samouri samouri added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 29, 2017
@bsessions85
Copy link
Contributor

E2E tests passed

@samouri samouri merged commit b1c4fef into master Nov 29, 2017
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 29, 2017
@sirreal sirreal deleted the framework/react16-take3 branch December 13, 2017 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants