Skip to content

Inline some internals, reduce shared/ utilities between isomorphic and renderers #9903

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

Merged
merged 11 commits into from
Jun 9, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jun 9, 2017

See individual commits.

The goal is to narrow down those utilities because they are copied into both bundles.
While we're at it, also making some things DEV-only.

There is a bit more duplication than I'd like, but it's partly due to Stack.
For things like Symbols, I don't think modules are super useful.

Note: this 441a04e breaks IE8 hard.

gaearon added 9 commits June 9, 2017 00:51
This breaks IE8. We don't support it.
It was added temporarily to avoid Stack shared state issues across renderers.
Not a problem anymore.
It's unlikely we'll deprecate anything else on React.* object soon.
It's only used in one place in isomorphic.
It's used more broadly in Stack so we move it there to die.
@@ -1991,10 +1992,6 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js
* can update text nodes when rendered as root
* can render and update root fragments

src/shared/utils/__tests__/KeyEscapeUtils-test.js
* should properly escape and wrap user defined keys
* should properly unescape and unwrap user defined keys
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't bother porting this test, for isomorphic (which is where this code leaves on) it's only testing flatten which is not even officially exposed. But I can write it against flatten specifically if somebody cares.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

😍

return '$' + escapedString;
}

var unescapeInDev = emptyFunction;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could just be null I guess. But doesn't matter since after Stack is deleted we can probably just inline this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's revisit when/if rewrite Children.

@gaearon gaearon merged commit 5c6a496 into facebook:master Jun 9, 2017
koba04 added a commit to koba04/react that referenced this pull request Jun 12, 2017
koba04 added a commit to koba04/react that referenced this pull request Jun 29, 2017
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.

3 participants