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

JavaScript helpers support on the server side #188

Closed
theotherian opened this Issue Jun 1, 2013 · 33 comments

Comments

Projects
None yet
5 participants
@theotherian
Copy link

theotherian commented Jun 1, 2013

Nashorn

dynjs

Currently, there's a disconnect in helper method implementations between client and server when using Handlebars.java in that methods implemented on the client would use JavaScript while methods implemented on the server would use Java.

While a helper method can be written in both languages, this requires additional maintenance, convention and oversight to accomplish successfully.

It's not uncommon for shops to have a front end development team who owns the view implementing helper methods in JavaScript, so there is clear value in having the same method exist in both the client and the server to allow full flexibility for rendering content.

jknack added a commit that referenced this issue Jun 3, 2013

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jun 3, 2013

done with Rhino and seems to be OK. We can live with this for now and keep exploring with nashorn or dynjs later

jknack added a commit that referenced this issue Jun 30, 2013

jknack added a commit that referenced this issue Jul 11, 2013

@jknack jknack closed this Jul 12, 2013

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jun 25, 2015

Hi Edgar,
did you per chance find the time in the meantime to further explore using Nashorn instead of Rhino?

The great thing about jknack is for us that it allows us to integrate frontends built with Grunt + Handlebars on the Java server-side, and they usually come with a number of helpers written in Javascript, such as #extends and #block.

We're now worried a bit about performance, as the JS helpers get executed on the server side with each request. https://blogs.oracle.com/nashorn/entry/nashorn_performance_work_in_the looks like Nashorn would provide a great performance improvement compared to Rhino (measured with the Octane benchmark).

Performance-wise, an even better alternative would be the V8 integration using JNI (http://eclipsesource.com/blogs/2014/11/17/highly-efficient-java-javascript-integration/). But I'm afraid of the complications coming with having to include binaries on the server.

Cheers,
Jörg

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jun 26, 2015

I didn't and the main concern around Nashorn is the JVM. If I add Nashorn support will force users to upgrade to Java 8, which won't be good for everyone.

Another option, will be to extract Rhino to it own module (maven project) and provide a new module for Nashorn. If I find some free time will try to do this.

when I did the JS integration via Rhino was also worry about the performance, but from what can I see... helpers got executed with a decent performance (may be not as fast as in Nashorn, but it is acceptable)

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jun 26, 2015

I understand, thanks for your answer!

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 8, 2015

I guess we then need to get rid of the enum com.github.jknack.handlebars.internal.JSEngine, as we cannot split the enum in two parts?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 8, 2015

When I move RhinoHandlebars into its own module, it starts complaining about imports from its former home:

import com.github.jknack.handlebars.Context;
import com.github.jknack.handlebars.Helper;
import com.github.jknack.handlebars.HelperRegistry;
import com.github.jknack.handlebars.Options;
import com.github.jknack.handlebars.internal.Files;
import com.github.jknack.handlebars.js.HandlebarsJs;

That seems to call for a handlebars-commons module or something like that, but I'm a little afraid that these classes will be dragging half of handlebars behind them into that module :-/

@jknack how do you think about this?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 8, 2015

OK I'll let handlebars-rhino and handlebars-nashorn depend on handlebars. But then anybody using handlebars.java will have to also explicitly declare a dependency to either handlebars-rhino or handlebars-nashorn.

Also running the tests will be a problem then. Adding a dependency with test-scope from handlebars to e.g. handlebars-rhino will make this a circular dependency, which calls for trouble.

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 8, 2015

it makes sense, yea. but it probably makes more sense to do a more portable version too.

for example, what if we just rely on javax.scripting and do something like:

try {
engine = new ScriptEngineManager().getEngineByName("nashorn")
} catch (Exception ex) {
  engine = new ScriptEngineManager().getEngineByName("rhino")
}

this also implies to remove the use/usage of rhino API.

thoughts?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 8, 2015

That's totally reasonable, as the Nashorn implementation would have to use javax.script API anyway I guess.

Do you per chance know of API incompatibilities off the top of your head that we might run into? Things like BetterNativeArray made me worry a little ;-)

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 8, 2015

yea, me too but we will have to play with it and probably come up with something done in pure js.

the good thing about betternativearray and company is that there are a few tests for that, but yea it is decent effort.

also, if we choose this path we won't have to split the js part to a new module.

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 9, 2015

hi @jknack
I came as far as getting rid of extending private API (NativeArray and NativeObject). The only test currently failing is this one: com.github.jknack.handlebars.js.JavaScriptHelperTest.hash()

Could you maybe tell me where the hash helper is defined?

Thx,
Jörg

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 9, 2015

And BTW, can we switch to source 1.7 instead of 1.6?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 9, 2015

... do you per chance have an Eclipse formatter file that I can use?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 9, 2015

thx!

Concerning Java 7: I really like diamond notation, Strings in switch statements, try-with-resources and multi-catch. It would be nice to be able to use them ...

I'd prefer to go with pull requests for the moment, thanks for asking!

(that checkstyle.xml surely isn't fun when writing delegates with all that redundant javadoc...)

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 9, 2015

Oh dear, I was assuming that ScriptableObject has a counterpart in javax.script, and successfully followed the approach in http://stackoverflow.com/a/7605787/1245428 , only to discover that there is no such counterpart in javax.script API.

I'm now thinking of using https://github.com/jdereg/json-io to convert the Java object graph to JSON, letting the JS engine evaluate that, and pass the result into JS. According to http://www.developer.com/lang/jscript/top-7-open-source-json-binding-providers-available-today.html , its performance seems to fare well in serialization of small data sets, and it sounds like it supports a lot of corner-cases, without requiring objects to implement Serializable.

WDYT?

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 9, 2015

is it just for rhino? or both?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 10, 2015

It's for both. I guess we cannot keep org.mozilla imports in classes being loaded into a Java 8 JVM?

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 10, 2015

Ok and no, we can't.

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 13, 2015

Jörg von Frantzius
jknack#188 use JSON to serialize complex objects over to JS
instead of using Native* API from org.mozilla package namespace

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 13, 2015

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

Would you mind dropping 1.6 support? Then we could drop the org.mozilla rhino dependency, as that doesn't implement any javax.script interfaces at all, and use Rhino bundled with Java 7 instead (and of course Nashorn bundled with Java 8).

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 14, 2015

Think we can move handlebars code base to be 1.7, but still compile/produce a 1.6 binary compatible.

then drop the rhino dep and use the embedded js engine, rhino for 1.6/1.7 and nashorn for 1.8

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

OK, let's hope Rhino exists as a maven artifact that can be added as a dependency when somebody wants to run on 1.6.

Another question: do we really need support for different handlebars versions handlebars-v2.0.0.js and handlebars-v1.3.0.js?

I'd like to precompile it to a CompiledScript, and supporting different versions would complicate this ...

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 14, 2015

1.x probably nope, but think we might want to support 2.x and 3.x.

All these will go into a major release for handlebars.java: 3.x so you do the work assuming we want the 3.x version of handlebars.js and precompile it to a CompiledScript

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

It seems I got it working, except for these two tests:

Failed tests:
Issue284.mustFailWhenCallToJavaScript Expected exception: java.lang.NoClassDefFoundError
Issue284.mustFailWhenUsingJsHelpers Expected exception: java.lang.IllegalStateException

I guess they just don't want to throw exception since Rhino is now always there :)
(They're contained in /handlebars-without-rhino)

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 14, 2015

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 14, 2015

Jörg von Frantzius
jknack#188 use only javax.script API
Assume either Rhino or Nashorn on the classpath, so drop old Rhino
dependency (which wasn't implementing javax.script); use CompiledScript
for precompilation of handlebars.js

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 14, 2015

Jörg von Frantzius
jknack#188 disable two tests that will never work on Java 7 and Java …
…8 now

because they expect failure due to missing rhino dependency
@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

I don't see the failures reported by https://travis-ci.org/jknack/handlebars.java/jobs/70948895 and https://travis-ci.org/jknack/handlebars.java/jobs/70948894 , and the difference is that I run on Oracle JDK, not OpenJDK.

I do see a single failure with Java 8:

Failed tests: 
  Issue243.zeroValueForJavaScriptHelper:30->AbstractTest.shouldCompileTo:29->AbstractTest.shouldCompileTo:43->AbstractTest.shouldCompileTo:63->AbstractTest.shouldCompileTo:81 '0.0 1.0 2.0 ' should === '0 1 2 ':  expected:<0[.0 1.0 2.0] > but was:<0[ 1 2] >

Tests run: 791, Failures: 1, Errors: 0, Skipped: 2

It seems Nashorn is more clever in not returning pesky doubles where where there are int values.

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 14, 2015

Jörg von Frantzius
jknack#188 cater for difference in rhino and nashorn within test
it seems Nashorn is more clever in not returning pesky doubles where
where there are int values
@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

I'm on Java version: 1.8.0_45 by the way, https://travis-ci.org/jknack/handlebars.java/jobs/70951353 is on java version "1.8.0_31"

That would be the only explanation I have why there are test failures on travis.

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 14, 2015

will try all these later today, but all the work you did is in #392 right? or are there more pulls coming?

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 14, 2015

Yes it's all in that pull request and there are no others coming currently. Cheers from Berlin!

@jknack

This comment has been minimized.

Copy link
Owner

jknack commented Jul 14, 2015

cool! will play and try later today.

thank you so much for being working with this!

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 16, 2015

In our project GSON seems to choke on circular references with java.lang.StackOverflowError :-/

@jfrantzius

This comment has been minimized.

Copy link

jfrantzius commented Jul 16, 2015

Turns out Java 8 Nashorn does provide nice object translation OOTB. This test succeeds in Java 8:

  @Test
  public void test() throws ScriptException {
    ScriptEngine jsEngine = new ScriptEngineManager().getEngineByName("JavaScript");
    Bindings bindings = jsEngine.createBindings();
    Map<String, Date> map = new HashMap<String, Date>();
    map.put("foo", new Date());
    bindings.put("mapAsObject", map);
    Object eval = null;
    eval = jsEngine.eval("mapAsObject.foo", bindings);
    Assert.assertEquals(map.get("foo"), eval);

    Object[] objectArray = new Object[1];
    objectArray[0] = new Object();
    bindings.put("objectArray", objectArray);
    eval = jsEngine.eval("objectArray[0]", bindings);
    Assert.assertEquals(objectArray[0], eval);

    List<Object> list = new ArrayList<Object>();
    list.add(new Object());
    bindings.put("listAsArray", list);
    eval = jsEngine.eval("listAsArray[0]", bindings);
    Assert.assertEquals(list.get(0), eval);

  }

The same test fails on Java 7, though.

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

Jörg von Frantzius
jknack#188 use Java 8 Nashorn built-in Java object to JS object trans…
…lation

Support Java 7 by invoking class via reflection from optional
handlebars-7 dependency

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

Jörg von Frantzius
jknack#188 use Java 8 Nashorn built-in Java object to JS object trans…
…lation

Support Java 7 by invoking class via reflection from optional
handlebars-java7 dependency

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

Jörg von Frantzius
jknack#188 use object translation in Nashorn, or own translation with…
… Rhino

By providing optional dependency handlebars-java7 that is used only via
reflection.

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

jfrantzius pushed a commit to aperto/handlebars.java that referenced this issue Jul 17, 2015

@vineet18

This comment has been minimized.

Copy link

vineet18 commented Aug 23, 2015

Hello @jfrantzius, @jknack,

Were you be able to get nashorn working with helpers on server-side? I am interested in the same since I am on a platform which used JDK1.8 and nashorn comes out of the box with it.

Appreciate quick help! Thanks

@matteosilv

This comment has been minimized.

Copy link

matteosilv commented Nov 14, 2016

@vineet18 I think you can already cloning the aperto/handlebars.java branch.
@jknack do you plan to merge @jfrantzius branch in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment