Don't try to make React elements immutable

[davidblurton]

Fixes #73

Allows you to map from an immutable to a React element.

[davidblurton]

Tests pass but error with

Error:
Zuul tried to run tests in saucelabs, however no saucelabs credentials were provided.
See the zuul wiki (https://github.com/defunctzombie/zuul/wiki/Cloud-testing) for info on how to setup cloud testing.

[shamrin]

@davidblurton Interesting approach to solving #73.

Have you considered simply checking for isReactComponent property? It was sugggested here:

https://discuss.reactjs.org/t/how-to-determine-if-js-object-is-react-component/2825/5

[davidblurton]

The property isReactComponent doesn't seem to be defined on ES6 classes.

To check for classes you'd have to do React.Component.isPrototypeOf(object) but that involves taking a dependency on react.

[shamrin]

You are right, React elements no longer can be detected by a simple property check.¹ Your approach seems to be the correct way. It's even mentioned in "React Components, Elements, and Instances" blog post:

All React elements require an additional $$typeof: Symbol.for('react.element') field declared on the object for security reasons.

¹ _isReactElement property was removed, at the same time as $$typeof was introduced - before React v0.14.0-rc1.

[shamrin]

What do you think about following React.isValidElement implementation more closely?

function isReactElement (obj) {
  return (
    typeof obj === 'object' &&
    obj !== null &&
    obj.$$typeof === REACT_ELEMENT_TYPE_FALLBACK || obj.$$typeof === REACT_ELEMENT_TYPE
  );
}

[davidblurton]

I initially just copied that function but I had it fail in production in IE11 because Symbol is defined but not implemented. Could also be an issue with the polyfill I was using.

It would be nice if it was the same implementation, but I couldn't get it to work like that. I need to write some tests for this. Do you think I should add a dev dependency on React so I can test the detection?

On 13 Apr 2016, at 22:18, Alexey Shamrin [email protected] wrote:

In src/seamless-immutable.js:

@@ -471,8 +471,17 @@
return makeImmutable(obj, mutatingObjectMethods);
}

• var REACT_ELEMENT_TYPE = typeof Symbol === 'function' && Symbol.for && Symbol.for('react.element');
• var REACT_ELEMENT_TYPE_FALLBACK = 0xeac7;

• function isReactElement(obj) {
• return obj.hasOwnProperty &&

•       obj.hasOwnProperty('$$typeof') &&
  

•       obj.$$typeof === REACT_ELEMENT_TYPE_FALLBACK || obj.$$typeof === REACT_ELEMENT_TYPE;
  

• }
  What do you think about following React.isValidElement implementation more closely?

function isReactElement (obj) {
return (
typeof obj === 'object' &&
obj !== null &&
obj.$$typeof === REACT_ELEMENT_TYPE_FALLBACK || obj.$$typeof === REACT_ELEMENT_TYPE
);
}
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[shamrin]

I think it should be okay in IE11. I've kept your …_TYPE_FALLBACK || …_TYPE condition intact.

The only difference between your isReactElement and React.isValidElement is that you test for hasOwnProperty, while they test for typeof obj and obj !== null.

Do you think I should add a dev dependency on React so I can test the detection?

Personally I don't see any problems with it. But take it with a grain of salt - I'm not a maintainer of this repo, @rtfeldman is.

[rtfeldman]

This is looking like the right approach! Would you mind adding some comments to the detection linking to where the magic constants come from?

I tried to avoid special casing React components for a long time, but the overwhelming demand has convinced me otherwise. One of the things this means is that now we need to document which React version(s) are supported, and the test cases will need to back that up.

So it probably won't be as simple as just adding React as a single dev dependency; since React will keep having new releases, there will pretty quickly need to be multiple React versions supported and tested separately!

[davidblurton]

Hopefully this isn't something that will change in React, since it was
added as a security feature. I'm not sure it's possible to depends on
multiple versions of the same package with npm, so might need to use
something else (bower?) to work around that.

I'll add some more comments and some tests for react 0.14 as a first pass.

I agree that it's strange for an immutability library to special case
react, but it does make working with the library more, well, seamless :)

On Thu, 14 Apr 2016 at 14:45 Richard Feldman [email protected]
wrote:

In src/seamless-immutable.js
#120 (comment)
:

@@ -471,8 +471,17 @@
return makeImmutable(obj, mutatingObjectMethods);
}

• var REACT_ELEMENT_TYPE = typeof Symbol === 'function' && Symbol.for && Symbol.for('react.element');
• var REACT_ELEMENT_TYPE_FALLBACK = 0xeac7;

• function isReactElement(obj) {
• return obj.hasOwnProperty &&

•       obj.hasOwnProperty('$$typeof') &&
  

•       obj.$$typeof === REACT_ELEMENT_TYPE_FALLBACK || obj.$$typeof === REACT_ELEMENT_TYPE;
  

• }

This is looking like the right approach! Would you mind adding some
comments to the detection linking to where the magic constants come from?

I tried to avoid special casing React components for a long time, but the
overwhelming demand has convinced me otherwise. One of the things this
means is that now we need to document which React version(s) are supported,
and the test cases will need to back that up.

So it probably won't be as simple as just adding React as a single dev
dependency; since React will keep having new releases, there will pretty
quickly need to be multiple React versions supported and tested separately!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/rtfeldman/seamless-immutable/pull/120/files/e5602f933b39e26c2f8ee78aaddd221745be27c8#r59730057

[rtfeldman]

Yeah, might need to get creative on the tests. React 0.14 is certainly a start!

[davidblurton]

I've added some tests for React elements and components. This along with the cycle detection should make things a lot smoother.

One problem I had was that the property that causes the issue, _owner, isn't filled in until the component is mounted in the dom. In other words, I was unable to reproduce the stack overflow in my tests. Instead I tested that React elements and components aren't modified.

CI build still failing with Zuul error.

[rtfeldman]

🎉 🎉 🎉 Thanks a ton @davidblurton!!!