zetachang / react.rb

Opal Ruby wrapper of React.js library.
http://reactrb.org/
MIT License
547 stars 35 forks source link

add svg support and impove the speed #118

Closed jiayp closed 8 years ago

ajjahn commented 8 years ago

@jiayp Can you tack on some specs the methods you added?

Also, I'm curious about the speed improvements. Do you have any benchmarks? I'd be cool to see where we should focus performance efforts.

jiayp commented 8 years ago

I'm a experienced ruby coder, but not for javscript and opal. I don't know how to write test/specs for opal ruby code.

In my project I found this result in chrome's profile before this changes

139.3 ms18.54 % 139.3 ms18.54 % (program) 
59.7 ms7.94 % 59.7 ms7.94 % react.self-eb8c71fecbd80164b57078dcf4c66c91c12c9c84b59938c485aea8b6ae3538c1.js:4025LinkedValueUtils.Mixin.propTypes.value 
42.1 ms5.61 % 42.1 ms5.61 % string.self-a6b99a3f5ef8ce2aef6253e5c54f5f3698453ec1be07b65b927bbd114ff88678.js:142def.$== 
42.1 ms5.61 % 42.1 ms5.61 % array.self-1d02b82cdc38e8a030fb2175a6c793817df26493d65c5ee2a2a2422d5466205b.js:1194def.$include? 
21.1 ms2.80 % 63.2 ms8.41 % array.self-1d02b82cdc38e8a030fb2175a6c793817df26493d65c5ee2a2a2422d5466205b.js:1194def.$include? 

The first one is couse by react's prop type check,and remove for production mode in react 0.14. The second is called by the third and called by HTML_TAGS.include?. The fourth is called by ATTRIBUTES.include?.

I rewrite the code using native js and the the second to fourth line disappeared from chrome's profile.

If need I can test again, and give the exactly result.

ajjahn commented 8 years ago

@jiayp

I'm a experienced ruby coder, but not for javscript and opal. I don't know how to write test/specs for opal ruby code.

The good news is, it is easy to learn because opal-rspec is pretty much exactly like ruby rspec. There's not really any JS required. You can write the tests in Ruby. We've already got a test suite that you can work with in the spec/ directory.

First, make sure you can run the existing tests:

  1. bundle install installs development dependencies.
  2. bundle exec rake test_app sets up testing environment.
  3. bundle exec rake runs the test suite.

One thing to pay attention to is which platform you are targeting for the test to run. If you are testing Ruby meant to be compiled by Opal into JS (which in this case you are), make sure your specs are inside an if opal? conditional. If you are testing plain Ruby, wrap your specs inside an if ruby? conditional. Have a look at some existing specs for an example.

If you'd like to take a stab at it, I'll help you if you hit any road blocks.

jiayp commented 8 years ago

OK I'll do it for svg. But how about performance?

ajjahn commented 8 years ago

You don't need to test the performance; just test that the added methods have the correct behavior.

jiayp commented 8 years ago

I got this error, how to fix it.

rake aborted!
Errno::ENOENT: No such file or directory - phantomjs
ajjahn commented 8 years ago

Ah, you need to install PhantomJS. If you are on a mac you can run brew install phantomjs. On other systems, you may need to start by reading the PhantomJS Getting Started guide.

jiayp commented 8 years ago

@ajjahn thanks, please check my pull request.

jiayp commented 8 years ago

I got this

Only PhantomJS v1 is currently supported,
if you're using homebrew on OSX you can switch version with:

  brew switch phantomjs 1.9.8

So I run brew install homebrew/versions/phantomjs198 and got

==> Installing phantomjs198 from homebrew/versions
phantomjs198: This formula either does not compile or function as expected on OS X
versions newer than Yosemite due to an upstream incompatibility.
jiayp commented 8 years ago

@ajjahn My commited code is tested by github ci.You can check it. My local environment dos not matter.

ajjahn commented 8 years ago

@jiayp Thanks for seeing this through!

If you're interested in getting the tests running locally, it looks like the method for installing PhantomJS on El Capitan is:

brew install npm npm install phantom phantomjs -g

It's discussed in https://github.com/Homebrew/homebrew/issues/44535.

sollycatprint commented 8 years ago

This issue was moved to reactrb/reactrb#118