| Thanks for the feedback zghst. Here's my response to your comments: > I'm sure it would help if these tests used the minified version of React on the server. Minification sometimes makes runtime performance worse due to the tricks that minifiers use to make code smaller. Even if it did help, it really only makes sense to run this benchmark against the unmodified react package installed from npm. If someone wants to look into make the react module run faster by modifying the source then that would be great, but I consider that outside the scope of this benchmark. > First (in SearchResults.jsx), setState accepts a callback in its second parameter, you don't need componentDidUpdate() or this.doneCallback (which is triggering more GC). Didn't notice that `setState` supported a callback. I made the change and it did not make any noticeable change in the benchmark (it only removed the need for a few extra assignments). NOTE: Adding a "this.doneCallback" is not trigging more GC because there is only one done callback every being created and it is common across both the Marko benchmark and the React benchmark. > Second in SearchResultsItem.jsx, you have an empty componentDidMount() function, you should get rid of that. Removing the empty function did make any noticeable difference in the numbers but I removed it anyway for those who want to re-run the tests on their own. > In SearchResultsItem.jsx, you should just go with a className (className={"search-results-item" + (this.state.purchased && ' user-has-purchased')}) instead of a style object, which becomes useless if you don't have a purchase. I used styles specifically because most React guides recommend using inline styles (not CSS class names). Also, as part of the benchmark I wanted to test behavior and I defined the behavior to be: "When the user clicks on the "Buy Now" button the search results item should turn yellow". I attempted to implement this in the most appropriate way for both Marko and React and I think I did it in a fair way. > By giving each SearchResultsItem their own state management, you're making it more dirty than you has to be, it should just be a simple list item. Typically in this type of app (Search), the SearchResultsItem component would be stateless (no this.state, getInitialState). When someone clicks handleBuyButtonClick, that communicates the change to the store, which then flows down to SearchResults, which "re-renders" the list of SearchResultsItems. What you are suggesting is the Flux way of updating the data store and view and I think that is a great approach. However, if I were to update this benchmark to do things in the Flux way then it would complicate the benchmark and it would make the React test slower given that it would try to re-render all of the search results items instead of just re-rendering the one search results item that was modified. |