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

Make ReactElement really a plain object and freeze it #3942

Merged
merged 1 commit into from
Sep 3, 2015

Conversation

sophiebits
Copy link
Collaborator

This should guarantee that the hidden class is the same after inlining.

(blocked on killing prop mutations)

@sophiebits sophiebits changed the title Make ReactElement really a plain object Make ReactElement really a plain object and freeze it Jun 10, 2015

props: props,

_isReactElement: true
Copy link
Member

Choose a reason for hiding this comment

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

,

@quantizor
Copy link
Contributor

Love this change. Would freezing props as well kill the blocker @spicyj?

@sophiebits
Copy link
Collaborator Author

@yaycmyk Yes, sorry – I meant that we needed to clean up some stuff internally that was still giving mutation warnings since the last release. I think that's all cleaned up now so this can go in.

@syranide
Copy link
Contributor

This has probably been discussed at length elsewhere, but I'm curious why we feel the need to freeze elements at creation, rather than freeze during hand-over (render) and allow modifications until then. That would make more sense to me. React requires it to be frozen as an API detail (but that could change...), but elements themselves are really just a special kind of object and them being frozen at creation by definition seems peculiar to me.

@quantizor
Copy link
Contributor

@syranide In my view it's simply a matter of ensuring total immutability & easy reasoning about changes. Once any data enters into a React render/composition flow, it should remain static and this type of change enforces that behavior from ears to toes.

@sophiebits
Copy link
Collaborator Author

My primary motivation for pushing for this change was #3226, which isn't possible if you allow mutation. But this also fits into a longer-term vision where JS has custom immutable value types that don't have real object identity and are compared by value. The language already has immutable numbers, booleans, and strings; the ReactElement type can be thought of as similar to those.

@syranide
Copy link
Contributor

@spicyj

My primary motivation for pushing for this change was #3226, which isn't possible if you allow mutation.

Yeah, or very complex at least (i.e. statically detect if it's never mutated or copy-on-write semantics).

But this also fits into a longer-term vision where JS has custom immutable value types that don't have real object identity and are compared by value. The language already has immutable numbers, booleans, and strings; the ReactElement type can be thought of as similar to those.

I'm not arguing against the idea of immutable elements, but all non-primitive types are mutable by default (i.e. objects), whereas elements are now immutable by default... it seems like an inconsistency and biased if we want to view JSX as a neutral language feature. Objects would benefit from the same optimization.

Anyway, I don't really care that much at this point, I'm just curious of the long-term.

@sebmarkbage
Copy link
Collaborator

@syranide The problem is that we can't reliably freeze them "later", since they're passed as children, props, nested inside of other objects, captured in closures etc.

There are languages that guarantee that an objects is frozen once it leaves the scope. JS is unfortunately not one of them. It makes subtyping difficult to reason about in type systems too. It's just more complex to reason about. It's simpler to just let things be born frozen, and we haven't really had a lot of complaints about this warning in 0.13.

This should guarantee that the hidden class is the same after inlining.
sophiebits added a commit that referenced this pull request Sep 3, 2015
Make ReactElement really a plain object and freeze it
@sophiebits sophiebits merged commit 4139b2e into facebook:master Sep 3, 2015
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