Add JRuby Support #79
Conversation
In face of JRuby issue to a particular MRI behavior involving lexical escope of `cvar`s of duped classes, this commit changes the approach by using instance vars without duping classes. Note: Even though it's a JRuby issue jruby/jruby#2988 (comment) I decided to change Lotus implementation because of the complexity of fixing it on JRuby side (second their core maintainers). Ref. hanami/hanami#307
|
@jodosha @joneslee85 need you help, guys, to review this :) |
|
Just for a record: The same issue has Rubinius as well. This is a specific MRI behaviour which is not covered by any specification. |
|
Thanks @deepj I think this fact scores on change the implementation :) |
| extend Rendering.dup | ||
| extend Inheritable | ||
| extend Dsl | ||
| extend Rendering | ||
| extend Escape.dup |
There was a problem hiding this comment.
This guy doesn't need it because it doesn't have class variables, so no issue with scope ;)
|
test passes it's a good starting point, AFAIK I think is ok but removing dup and class I dont the new behaviour. @jodosha could you take a look? |
|
@AlfonsoUceda I think that is a similar behavior, instead of maintaining cvars and duping classes we're keeping instance vars with new instances. Be aware that the objects are still being frozen and they still keep using the same reference like the previous approach |
|
👍 to me cc @jodosha |
|
👍 |
|
I can benchmark it :) Do you guys have any "official" benchmark strategy? |
|
@brennovich please talk to @jodosha on Gitter, I believe he's already had a set of benchmark scripts |
|
Well, discussing with @jodosha he told me about his concerns about the Setuplotus new bookshelf --lotus-head=true
cd bookshelf
vim Gemfile # point lotus-view to your branch
bundle
bundle exec lotus generate app adminCurrent Implementation
This branch
We can see that configurations keep themselves valid without leaking (look at te irb(main):009:0> Web::Application.new
=> #<Web::Application:0x10db6131 @renderer=#<Lotus::RenderingPolicy:0x4730e0f0 @templates=<Pathname:/home/brennovich/code/bookshelf/apps/
web/templates_web>, @view_pattern="Views::%{controller}::%{action}", @namespace=Web, @controller_pattern=/Controllers::(?<co
ntroller>(.*))::(?<action>(.*))/>>
irb(main):010:0> Admin::Application
=> Admin::Application
irb(main):011:0> Admin::Application.new
=> #<Admin::Application:0x37c41ec0 @renderer=#<Lotus::RenderingPolicy:0x5399f6c5 @templates=<Pathname:/home/brennovich/code/bookshelf/apps/
admin/templates>, @view_pattern="Views::%{controller}::%{action}", @namespace=Admin, @controller_pattern=/Controllers::(?<co
ntroller>(.*))::(?<action>(.*))/>>
irb(main):012:0> Web::Application.new
=> #<Web::Application:0x76464795 @renderer=#<Lotus::RenderingPolicy:0x48f4713c @templates=<Pathname:/home/brennovich/code/bookshelf/apps/
web/templates_web>, @view_pattern="Views::%{controller}::%{action}", @namespace=Web, @controller_pattern=/Controllers::(?<co
ntroller>(.*))::(?<action>(.*))/>> |
| @@ -16,13 +24,11 @@ rvm: | |||
| - 2.2.1 | |||
| - 2.2.2 | |||
| - 2.2.3 | |||
| - jruby-9000 | |||
There was a problem hiding this comment.
please test against newer version 9.0.1.0
There was a problem hiding this comment.
I was using 9.0.1.0 on my machine, but when I wrote this PR rvm wasn't with this version available :/
There was a problem hiding this comment.
There was a problem hiding this comment.
Yay! I'll open a PR with adding this ;)
|
@brennovich looks good, please rebase |
|
@brennovich @joneslee85 I manually tested and can confirm this works 👍 I'll take care of merge it. 👍 |
|
Thanks for testing it @jodosha :) |
In face of JRuby issue to a particular MRI behavior involving lexical
escope of
cvars of duped classes, this commit changes the approach byusing instance vars without duping classes.
Note: Even though it's a JRuby issue
jruby/jruby#2988 (comment)
I decided to change Lotus implementation because of the complexity of
fixing it on JRuby side (second its core maintainers).
Ref. hanami/hanami#307