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 svg support and impove the speed #118

Closed
wants to merge 6 commits into from
Closed

Conversation

jiayp
Copy link

@jiayp jiayp commented Jan 14, 2016

No description provided.

@ajjahn
Copy link
Collaborator

ajjahn commented Jan 14, 2016

@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
Copy link
Author

jiayp commented Jan 14, 2016

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
Copy link
Collaborator

ajjahn commented Jan 14, 2016

@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
Copy link
Author

jiayp commented Jan 15, 2016

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

@ajjahn
Copy link
Collaborator

ajjahn commented Jan 15, 2016

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

@jiayp
Copy link
Author

jiayp commented Jan 15, 2016

I got this error, how to fix it.

rake aborted!
Errno::ENOENT: No such file or directory - phantomjs

@ajjahn
Copy link
Collaborator

ajjahn commented Jan 15, 2016

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
Copy link
Author

jiayp commented Jan 15, 2016

@ajjahn thanks, please check my pull request.

@jiayp
Copy link
Author

jiayp commented Jan 15, 2016

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
Copy link
Author

jiayp commented Jan 15, 2016

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

@ajjahn ajjahn closed this in 4408438 Jan 15, 2016
@ajjahn
Copy link
Collaborator

ajjahn commented Jan 15, 2016

@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 Homebrew/legacy-homebrew#44535.

@sollycatprint
Copy link

This issue was moved to ruby-hyperloop/hyper-react#118

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants