Use hydrate() instead of render() starting from react 16 #30

wingleung · 2017-10-10T07:27:31Z

PR for #27

goatslacker · 2017-10-10T17:14:11Z

package.json

    "sinon-sandbox": "^1.0.2"
  },
+  "dependencies": {
+    "semver": "^5.4.1"


1200 LOC just so we can support both Reacts at the same time? I'd rather semver breaking and backport fixes. Having two different branches is not worth it IMO.

Another alternative is letting the default entry point be "render" and providing a new entry point for "hydrate".

created a separate function for hydrate

Another alternative is calling hydrate when it's present, and otherwise just fall back to render - checking the React version is brittle anyways

ljharb · 2017-10-11T19:36:29Z

package.json

    "hypernova": "^2.0.0",
-    "react": "0.14.x || >= 15.x",
-    "react-dom": "0.14.x || >= 15.x"
+    "react": "0.14.x || >= 15.x || >= 16.x",


>= 15.x actually already includes 16.x, so these lines don't really need to change (altho they should probably change to ^0.14 || ^15 || ^16 instead of using >=

@ljharb done, however I'm not sure why this was even necessary from the start. Couldn't we just do >0.14? or do we have to ignore 0.15? In that case we should probably use ~0.14 instead

updated to use ~0.14, I suspect we have to ignore 0.15 for some reason?

There is no 0.15.

ljharb · 2017-10-12T20:51:14Z

package.json

-    "react": "0.14.x || >= 15.x",
-    "react-dom": "0.14.x || >= 15.x"
+    "react": "~0.14 || ^15 || ^16",
+    "react-dom": "~0.14 || ^15 || ^16"


^ for versions below 1.0 only updates the patch, so ^0.14 is correct here.

updated, thanks for bearing with me on this. Really, I'm gaining knowledge here!

ljharb

LGTM, thanks!

We should probably try to add some kind of test; the test would probably just assert that when ReactDOM.hydrate was a function, it called it; else it called render.

wingleung · 2017-10-13T20:42:44Z

@ljharb Added tests

ljharb · 2017-10-13T22:47:52Z

test/renderReact-test.js

      global.window = window;
      global.document = window.document;

+      const hydrateMethod = sinon.spy(ReactDOM, 'hydrate');


will this work when hydrate doesn't exist yet?

@ljharb yes, this test runs in the hypernova-react package which (in this PR) depends on React ^16 and therefore always has the hydrate method available.
https://github.com/airbnb/hypernova-react/pull/30/files

hypernova-react doesn't depend on v16, it devDepends on it, and peerDepends on 0.14 or 0.15 or 16 - if we wanted to test this repo on 0.14 or 0.15 (which we should, if we're not already), this test would fail.

how will we switch the react version to in our test cases? Shouldn't we use v16 and mock v15/v0.14 behaviour, which I'm doing on line 59

I think this is fine for this PR :-) when we actually add tests on all the React versions, we wouldn't want to mock things out - mocks make tests more brittle.

sebjacobs · 2017-11-17T09:30:27Z

Are there any plans to merge this PR?

ljharb

Let's make this last change, and rebase, and this will be good to go.

ljharb · 2017-11-17T15:33:40Z

package.json

    "mocha": "^4.0.0",
    "prop-types": "^15.6.0",
-    "react": "^15.6.1",
-    "react-dom": "^15.6.1",


devDep and peerDep ranges should all be identical; let's make those the same

wingleung · 2017-11-22T15:15:57Z

package.json

-    "react": "^15.6.1",
-    "react-dom": "^15.6.1",
+    "react": "^0.14 || ^15 || ^16",
+    "react-dom": "^0.14 || ^15 || ^16",


@ljharb like this? when I test this it always installs v16

yep! that's fine, it will always install the latest, but it will be valid to install older versions.

ljharb · 2017-11-22T15:52:22Z

@wingleung last thing needed is to rebase on latest master; i'm happy to do it for you if needed.

wingleung · 2017-11-22T16:49:11Z

@ljharb rebase done

- [New] Use hydrate when possible (#30)

[New] Use hydrate when possible

82b92ba

wingleung changed the title ~~Usa hydrate instead of render starting from react 16~~ Use hydrate() instead of render() starting from react 16 Oct 10, 2017

goatslacker suggested changes Oct 10, 2017

View reviewed changes

wingleung force-pushed the feature/hydration-instead-of-render branch 3 times, most recently from f827c18 to a22c131 Compare October 11, 2017 18:00

ljharb suggested changes Oct 11, 2017

View reviewed changes

wingleung force-pushed the feature/hydration-instead-of-render branch 2 times, most recently from e3ce178 to e5bc6aa Compare October 12, 2017 18:08

ljharb suggested changes Oct 12, 2017

View reviewed changes

wingleung force-pushed the feature/hydration-instead-of-render branch from e5bc6aa to 7c68975 Compare October 13, 2017 07:07

ljharb approved these changes Oct 13, 2017

View reviewed changes

wingleung force-pushed the feature/hydration-instead-of-render branch from 7c68975 to 5cc94b6 Compare October 13, 2017 20:38

goatslacker approved these changes Oct 13, 2017

View reviewed changes

ljharb reviewed Oct 13, 2017

View reviewed changes

ljharb suggested changes Nov 17, 2017

View reviewed changes

wingleung commented Nov 22, 2017

View reviewed changes

wingleung force-pushed the feature/hydration-instead-of-render branch from fc218f9 to c00c0e0 Compare November 22, 2017 16:28

ljharb force-pushed the feature/hydration-instead-of-render branch from c00c0e0 to 82b92ba Compare November 22, 2017 16:54

ljharb approved these changes Nov 22, 2017

View reviewed changes

ljharb added the enhancement label Nov 22, 2017

ljharb merged commit 82b92ba into airbnb:master Nov 22, 2017

ljharb mentioned this pull request Nov 22, 2017

Use hydrate() instead of render() #27

Closed

ljharb added a commit that referenced this pull request Nov 30, 2017

v2.1.0

83ef218

- [New] Use hydrate when possible (#30)

Use hydrate() instead of render() starting from react 16 #30

Use hydrate() instead of render() starting from react 16 #30

Uh oh!

Conversation

wingleung commented Oct 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wingleung Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

wingleung commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebjacobs commented Nov 17, 2017

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb commented Nov 22, 2017

Uh oh!

wingleung commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wingleung Oct 12, 2017 •

edited

Loading