-
Notifications
You must be signed in to change notification settings - Fork 93
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
Upgrade to libv8 8.4 #166
Upgrade to libv8 8.4 #166
Conversation
This is fantastic @nightpool ... @ignisf can we merge this? |
Unfortunately i'm still having problems with my React SSR related to locale data. I'm working on a further patch for libv8, but my current working theory is that libv8 isn't packaging locale data at all. Adding it statically to the v8 |
I would say it depends on how much extra size would be added by adding locale data? |
Okay, it looks like the locale data isn't too much bigger—my libv8 .gem went from 11.9 MB to 16.2 MB. Not as bad as I was expecting, but still a pretty big increase. |
Will leave this for @ignisf to decide but my gut feel is that it is OK. Is there are way to package it so it is an additional dependency eg: |
I haven't looked into it yet. I'm sure it's possible, not sure what the additional complexity would be or how long it would take to get working. |
Wait this isn't going to work because i don't have darwin binaries. Oh well. |
@nightpool shall I merge this now? is is backwards compat? |
@SamSaffron I haven't investigated the c++ changes in detail, because I was mostly building on the work @cataphract and @ignisf did, but my guess is probably not. (especially since I bumped the gemfile to require libv8 8.4, which hasn't been released yet) |
K ... going to hold off on merging this till we get libv8 8.4 published. |
@SamSaffron @cowboyd @ignisf built libv8 gems are ready at https://github.com/rubyjs/libv8/releases/tag/v8.4.255.0, I've tested them locally and we're just waiting on getting them pushed to rubygems before we can merge this! |
Amazing work @nightpool I just pushed a release to rubygems! merging this in! |
yes!!!!!!!! so great to get this merged. |
Based on #163 with a slight tweak to the gemspec and tests. Requires rubyjs/libv8#301 to be merged.
Future versions of v8 (8.4 stable, 8.5 beta, and 8.6 development) are blocked on https://bugs.chromium.org/p/v8/issues/detail?id=10041. Right now you can test locally by installing https://github.com/nightpool/libv8/packages/314620, but be aware that I've only pushed a version for
x86_64-linux
, since that's what I was testing on (specifically, i tested on theruby:2.5.8-stretch
docker container).rake test
passes locally, I haven't tried any of the other tests yet.closes #153, #163