Align with autobinding changes in React master

[gaearon]

There were some changes to autobinding internal field names.
We should support the new field names in master: facebook/react#5550

[gaearon]

@akorchev Would you like to take a look at this?

[gaearon]

Relevant part starts with

https://github.com/facebook/react/pull/5550/files#diff-d57db055bf32620520a16ab16dc6ec2fL702

We should support both old and new style.

[akorchev]

I will take a look. Not sure how to test it though. I guess I will check the current implementation around __reactAutoBindMap.

[gaearon]

Testing is tricky. I think our best bet is to test it in a branch with existing tests (but using 0.15 alpha), later transition to testing with 15 in master when it’s stable.

[akorchev]

I could do that but I don't understand what __reactAutoBindMap originally did. Are there any tests in react-proxy that rely on that behavior?

[gaearon]

I think if you remove the relevant code then createClass() tests will fail. I don’t fully remember why __reactAutoBindMap is necessary in React, but the idea is that React calls bindAutoBindMethods when creating instances, and we copy-paste that function to replicate the same behavior for any newly added methods so they also get autobound. In our case, we’d need to add something like bindAutoBindMethods15 that replicates what React does in the source in v15, and conditionally call either depending on which map exists.

[gaearon]

To clarify, this:

react-proxy/src/bindAutoBindMethods.js

Lines 13 to 54 in 4bdc8b3

	function bindAutoBindMethod(component, method) {
	var boundMethod = method.bind(component);
	boundMethod.__reactBoundContext = component;
	boundMethod.__reactBoundMethod = method;
	boundMethod.__reactBoundArguments = null;
	var componentName = component.constructor.displayName,
	_bind = boundMethod.bind;
	boundMethod.bind = function (newThis) {
	var args = Array.prototype.slice.call(arguments, 1);
	if (newThis !== component && newThis !== null) {
	console.warn(
	'bind(): React component methods may only be bound to the ' +
	'component instance. See ' + componentName
	);
	} else if (!args.length) {
	console.warn(
	'bind(): You are binding a component method to the component. ' +
	'React does this for you automatically in a high-performance ' +
	'way, so you can safely remove this call. See ' + componentName
	);
	return boundMethod;
	}
	var reboundMethod = _bind.apply(boundMethod, arguments);
	reboundMethod.__reactBoundContext = component;
	reboundMethod.__reactBoundMethod = method;
	reboundMethod.__reactBoundArguments = args;
	return reboundMethod;
	};
	return boundMethod;
	}
	export default function bindAutoBindMethods(component) {
	for (var autoBindKey in component.__reactAutoBindMap) {
	if (!component.__reactAutoBindMap.hasOwnProperty(autoBindKey)) {
	return;
	}

is pretty much copy-pasted from React source.

We just need to copy-paste the new version of these methods and switch based on the instance field we detect.

[akorchev]

Yep. Pretty much everything fails after upgrading to 0.15 alpha. Will see If I can fix that.

[gaearon]

Thanks for taking a look. If it doesn’t work out, ping me and I can try to help.

[akorchev]

Is it just me or this code never runs in tests? https://github.com/gaearon/react-proxy/blob/master/src/createClassProxy.js#L160

When I ran them against 0.15 (in this branch) bindAutoBindMethods never runs. Which makes me think that the tests fail for other reasons.

The tests first failed with with this stack trace:

TypeError: Cannot read property 'toString' of undefined
      at getReactRootIDString (node_modules/react/lib/ReactInstanceHandles.js:32:27)
      at Object.ReactInstanceHandles.createReactRootID (node_modules/react/lib/ReactInstanceHandles.js:188:12)
      at [object Object].ReactShallowRenderer._render (createShallowRenderer.js:97:39)
      at [object Object].ReactShallowRenderer.render (createShallowRenderer.js:78:8)
      at Context.<anonymous> (unmounting.js:123:38)

It seems createReactRootID now needs a parameter. After calling it with an argument I ended up with
'TypeError: transaction.getReactMountReady is not a function'

Could there be a rendering problem in the tests with 0.15?

[akorchev]

The tests now pass in 0.15. However to make it work in 0.14 I need to somehow detect the react version because of this line: https://github.com/gaearon/react-proxy/blob/auto-bind/test/helpers/createShallowRenderer.js#L102
@gaearon ideas? I can rely on React.version or maybe check the signature of the mountComponent function?

[gaearon]

If it works in 0.15 we can just use it for testing instead (and modify shallow renderer code accordingly). I copied shallow renderer from React code and made a few tiny changes for my use case but it changed since then.

[gaearon]

So we can fix autobinding in 15, apply that fix to our master branch (which wouldn't really be tested but we'd know it works and be careful not breaking it) and release both 1.x and 2.x, and keep 15 branch around where we actually test with 15. When 15 is released, we can just use it in master tests instead.

[gaearon]

Ideally of course we should test multiple versions of React but ain’t nobody got time for that.

[akorchev]

Sounds like a plan. I will rename the branch to 0.15 and cherry-pick everything but package.json and the shallow renderer. OK?

[akorchev]

@gaearon too afraid to push before a review so opened two PRs.

[gaearon]

These look great. Can you verify whether they work with React master? I don't think this code changed since 0.15 alpha but still worth checking before we cut a release.

[akorchev]

The tests pass with React master. Out of curiosity what is the most efficient way to test against React master? I cloned the repo, built the repo and then copied it over node_modules/react/lib. There must be a better way.

[gaearon]

Maybe there is but that is what I do too.

[akorchev]

I usually do npm install org/repo but it didn't work for react. The root package.json is for 'react-build' whatever that is.

[gaearon]

Manually tested, and this appears to be fixed. Much appreciated!

[akorchev]

Don't mention it. I am happy to help.