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

Add support for client-side batching to generate fewer <style> tags. #1

Merged
merged 1 commit into from
Oct 27, 2015

Conversation

jlfwong
Copy link
Collaborator

@jlfwong jlfwong commented Oct 9, 2015

This also adds some examples to the repo, and paves the path forward for serverside rendering.

Test Plan:

cd examples
npm install
npm run examples

Open http://localhost:4114/, see 2 style tags generated: one for initial render, one for the dynamic blue added by the timer.

@xymostech

<div id='root'>{html}</div>
<script src="./bundle.js"></script>
<script>StyleSheet.markInjected({classNames});</script>
</body>
Copy link
Contributor

Choose a reason for hiding this comment

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

<script>ReactDOM.render(<App />, document.querySelector("#root"))</script>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't that strictly worse than document.getElementById? Seems like querySelector should just be slower because it needs to the the string parser to figure out to search by id?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like yes: https://jsperf.com/getelementbyid-vs-queryselector
I mostly wanted to mention to add the corresponding client-side ReactDOM.render call.

injectionBuffer = "";
},

css(styleDefinitions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed, correct? (since it's now merged into css below?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, yep -- bad merge

@xymostech
Copy link
Contributor

This all looks fine to me (except for the function to be removed)!

This also adds some examples to the repo, and paves the path forward for serverside rendering.

Test Plan:

    cd examples
    npm install
    npm run examples
    Open http://localhost:4114/, see 2 style tags generated: one for initial render, one for the dynamic blue added by the timer.
jlfwong added a commit that referenced this pull request Oct 27, 2015
Add support for client-side batching to generate fewer <style> tags.
@jlfwong jlfwong merged commit 2c6c06c into master Oct 27, 2015
@jlfwong jlfwong deleted the jlfwong-batching branch October 27, 2015 21:49
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