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 GraalJS as Javascript engine in F!F, next to J2V8 #6834

Merged
merged 18 commits into from
May 27, 2024

Conversation

jkosternl
Copy link
Contributor

@jkosternl jkosternl commented May 16, 2024

Remark: the unit test JavascriptSenderTest.testWithJavaScriptReturningAnObject has different results with GraalJS. The return types are different and uses internal GraalJS objects, which are not always accessible.

@jkosternl jkosternl self-assigned this May 16, 2024
@jkosternl jkosternl linked an issue May 16, 2024 that may be closed by this pull request
Comment on lines 285 to 286
Arguments.of(JavaScriptEngines.GRAALJS, "returnObject", "{answer: 42}"), // Don't understand why this is different in GraalJS
Arguments.of(JavaScriptEngines.GRAALJS, "returnArray", "(3)[1, 3, 5]")
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

De bovenste is goed, het is fijn dat hij nu Json responses terug kan geven. Maar de array had ik anders verwacht...
Zou het mogelijk zijn die (3) er van af te snoepen en iets terug te geven waar de eindgebruiker wat meer mee kan?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ik vraag me af of die (3) gerelateerd is aan de lengte van de array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ja, dat is hij zeker. Ik heb ook de toString gevonden in de Javascript engine die dit doet. Ik ben bezig geweest met type mappers om te zorgen dat hij zich anders gedraagt. Helaas werd dat nog niet goed opgepakt.

@nielsm5
Copy link
Sponsor Member

nielsm5 commented May 17, 2024

Remark 2: the unit test JavascriptSenderTest.testWithJavaScriptReturningAnObject has different results with GraalJS. I don't fully understand why. Please let me know if that's Ok.

If would prefer a more useable response (without the (3)[...

Question: shall I also add the new engine to AppConstants.properties:
flow.javascript.engines=org.frankframework.javascript.J2V8,org.frankframework.javascript.GraalJS

If you can verify that it works on your machine (as in, does the FlowGenerator work with GraalJS, and are the flows visible in the console).

Question: should I also duplicate some Larva tests to GraalJS? Or move 2 tests to GraalJS instead of J2V8? Can also move all of them and make GraalJS the default? Many options 😉

You most certainly could! I don't think it's wise to duplicate all the tests, but perhaps changing a few would be a good start!

@jkosternl
Copy link
Contributor Author

Tried to address 2 things: type issue & temp dir issue while starting in Tomcat & Docker. No solution yet.
The current experiments are in this patch:
Experiments.patch

Refs:
oracle/graaljs#94
https://stackoverflow.com/questions/68844706/graalsdk-value-as-json-structure
oracle/graal#8222

@nielsm5
Copy link
Sponsor Member

nielsm5 commented May 21, 2024

It seems that the polyglot.engine.userResourceCache will be introduced in version 24.1.0, to be released in 17 September 2024. (see https://www.graalvm.org/release-calendar/)

@jkosternl jkosternl marked this pull request as draft May 22, 2024 08:07
public void registerCallback(final ISender sender, final PipeLineSession session) {
scriptEngine.put(sender.getName(), (JavaCallback) s -> {
try {
Message msg = Message.asMessage(s[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moet deze msg niet ook worden gesloten in een try-with-resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goed punt, ik heb het zojuist geprobeerd. Echter blijft de promiseResult in org.frankframework.senders.JavascriptSenderCallbackTest#promise steeds leeg en klopt het resultaat niet. Ik begrijp niet helemaal waarom dat is, maar iig krijg ik het niet werkend. Het is ook niet erg dat hij geen close krijgt, omdat het request toch van het type String is.

docker/Tomcat/webapp/Dockerfile Outdated Show resolved Hide resolved
@jkosternl jkosternl marked this pull request as ready for review May 23, 2024 13:29
@jkosternl jkosternl requested review from nielsm5 and tnleeuw May 24, 2024 12:03
Copy link

sonarcloud bot commented May 25, 2024

@nielsm5 nielsm5 merged commit 38619e7 into master May 27, 2024
16 of 17 checks passed
@nielsm5 nielsm5 deleted the feature/5496_AddJSEngine branch May 27, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new Javascript Engine
4 participants