Skip to content

GraalJS Compatibility #156 - #157

Merged
rymsha merged 5 commits into
masterfrom
graaljs-compatibility
Aug 4, 2026
Merged

GraalJS Compatibility #156#157
rymsha merged 5 commits into
masterfrom
graaljs-compatibility

Conversation

@rymsha

@rymsha rymsha commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #156.

What

Nashorn maps a property write on a Java host object to its setter (bean.foo = xsetFoo(x)); GraalJS does not — that shorthand went away with nashorn-compat mode (enonic/xp#9236). Every such write throws TypeError: writeMember (foo) ... Unknown identifier: foo at runtime.

This replaces all 38 property writes listed in the issue with the matching setter calls in src/main/resources/lib/text-encoding.js, each verified against its handler class:

Property write Setter Handler(s)
bean.stream = … bean.setStream(…) Base64 / Base64Url / Base32 / Hex / CharsetEncoding / HashFunction / HmacFunction
bean.text = … bean.setText(…) Base64 / Base64Url / Base32 / Hex / CharsetEncoding / UrlEscape / HtmlEscape / XmlEscape
bean.charset = … bean.setCharset(…) CharsetEncoding (field is charsetName, setter is setCharset)
bean.key = … bean.setKey(…) HmacFunction

setStream takes Object (the Java side converts strings/numbers/booleans), so the stream sites pass the value straight through. setKey is guarded by an explicit key === undefined throw, so it needs no null wrap.

String coercion for setText

One nuance beyond the mechanical setter conversion: Nashorn also coerced number/boolean values to String when writing a String-typed property, so urlEscape(33)"33" and urlEscape(true)"true". The plain setText(33) call does not — GraalJS rejects it with "Cannot convert '33' (…) to Java type 'java.lang.String': Invalid or lossy primitive coercion" — which broke the pre-existing testUrlEscapeNumber / testUrlEscapeBool tests.

To preserve that behaviour on both engines, the text setters receive a null-safe String(...):

bean.setText(text == null ? null : String(text));

This is a superset of the __.nullOrValue the issue prescribes — same undefined/nullnull handling, plus the coercion Nashorn's property write used to perform. The charset setter keeps __.nullOrValue(charset) (a charset is a name, never a non-string).

Test on both engines

Adds a testGraalJs task (mirroring the platform's gradle/js-tests.gradle) that runs the existing suite under the GraalJS engine, wired into check. It needs the ScriptRunnerSupport fix from enonic/xp#12198, so xpVersion is bumped to 8.1.0-SNAPSHOT and the repo is switched to xp.enonicRepo('dev').

Local verification

Built the platform branch enonic/xp@claude/graaljs-support-limitations-pzu6z8 (which carries #12198) to mavenLocal, added mavenLocal() locally, then ran ./gradlew test testGraalJs:

  • before the fix (property writes, GraalJS) — testGraalJs fails, e.g. PolyglotException at .../text-encoding.js:18
  • after the fix — both engines green:
    • test (Nashorn): 82 tests, 0 failed
    • testGraalJs (GraalJS): 82 tests, 0 failed

CI status

⚠️ testGraalJs will stay red in CI until enonic/xp#12198 is merged and a fresh 8.1.0-SNAPSHOT is published. The snapshot currently on repo.enonic.com/dev still carries the old ScriptRunnerSupport, which closes the script executor during test discovery. This PR is kept draft until then; the Nashorn test task stays green.

Nashorn mapped a property write on a Java host object to its setter
(bean.foo = x -> setFoo(x)); GraalJS does not, so every such write now
throws "TypeError: writeMember ... Unknown identifier" at runtime.
Replace all 38 property writes in lib/text-encoding.js with the matching
setter calls, each verified against its handler class.

Nashorn also coerced number/boolean values to String when writing a
String-typed property; the plain setter call does not, which broke the
existing urlEscape(33)/urlEscape(true) tests. Feed the text setters a
null-safe String(...) so that coercion is preserved on both engines.

Add a testGraalJs task (org.graalvm.polyglot:js) wired into check that
runs the existing suite under the GraalJS engine, mirroring the
platform's gradle/js-tests.gradle. Requires xpVersion=8.1.0-SNAPSHOT
from the dev repo.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rymsha

rymsha commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

CI ran on 651cf77ed3e5fed063850dbaee85359997ac4829run 30430108773:

  • :test (Nashorn) — green
  • :testGraalJs (GraalJS) — 82 tests, 82 failedBUILD FAILED

That 82/82 is the pre-#12198 signature: the 8.1.0-SNAPSHOT currently on repo.enonic.com/dev still carries the old ScriptRunnerSupport, which closes the script executor during test discovery, so every spec fails before it runs. Against a local platform build that includes enonic/xp#12198, all 82 pass on both engines. Kept draft until a fresh snapshot with #12198 is published; the red build check is expected until then.

@rymsha
rymsha requested a review from anatol-sialitski August 3, 2026 06:45
Comment thread src/main/resources/lib/text-encoding.js Outdated
com.enonic.xp 4.2.0 generates the per-engine test tasks from
xp.scriptEngines, and testing:8.1.0-SNAPSHOT brings the GraalJS engine
transitively, so the hand-rolled testGraalJs task and the graalvm.js
dependency are no longer needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.57%. Comparing base (5613d07) to head (afc639c).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #157   +/-   ##
=========================================
  Coverage     96.57%   96.57%           
  Complexity       75       75           
=========================================
  Files            11       11           
  Lines           146      146           
  Branches          4        4           
=========================================
  Hits            141      141           
  Partials          5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rymsha
rymsha marked this pull request as ready for review August 4, 2026 05:26
rymsha and others added 3 commits August 4, 2026 08:16
Addresses review: the explicit String(text) coercion is unnecessary and
the simpler __.nullOrValue(text) mirrors how charset is already passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg
__.nullOrValue(text) breaks testUrlEscapeBool/testUrlEscapeNumber under
GraalJS: unlike Nashorn, GraalJS will not coerce a JS boolean/number
into the Java String parameter of setText, so the explicit
String(text) is required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg
The *Decode functions take an encoded string by contract, so the
explicit String() coercion is unnecessary there (per review). The
escape/unescape functions and charsetEncode keep it: they accept
arbitrary values, and GraalJS will not coerce a JS number/boolean into
the Java String setter (see testUrlEscapeBool/testUrlEscapeNumber).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg
@rymsha
rymsha merged commit 7cdccd1 into master Aug 4, 2026
4 checks passed
@rymsha
rymsha deleted the graaljs-compatibility branch August 4, 2026 06:29
rymsha added a commit to enonic/app-simple-idprovider that referenced this pull request Aug 4, 2026
The released 3.0.0 uses Nashorn-style bean property assignment
(bean.stream = stream) which GraalJS rejects, failing the two-step
login tests under testGraalJS. The 3.0.0-SNAPSHOT build carries the
setter-based fix (enonic/lib-text-encoding#157).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg
rymsha added a commit to enonic/app-simple-idprovider that referenced this pull request Aug 4, 2026
* GraalJS Compatibility #102

Nashorn maps `bean.email = x` to `setEmail(x)`; GraalJS does not (the
nashorn-compat shorthand was removed in enonic/xp#9236), so the property
write in lib/gravatar.js throws
`TypeError: writeMember (email) ... Unknown identifier: email` at runtime.
Call the setter instead.

Add the dual-engine test setup (testRuntimeOnly org.graalvm.polyglot:js and
a testGraalJs task wired into check), a script-level test that exercises
gravatar.hash on both engines, and bump xpVersion to 8.1.0-SNAPSHOT (the
ScriptRunnerSupport fix from enonic/xp#12198 that testGraalJs needs).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Use plugin script-engine matrix instead of custom GraalJS harness

com.enonic.xp 4.2.0 generates the per-engine test tasks from
xp.scriptEngines, and testing:8.1.0-SNAPSHOT brings the GraalJS engine
transitively, so the hand-rolled testGraalJs task and the graalvm.js
dependency are no longer needed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg

* Use lib-text-encoding 3.0.0-SNAPSHOT for GraalJS compatibility

The released 3.0.0 uses Nashorn-style bean property assignment
(bean.stream = stream) which GraalJS rejects, failing the two-step
login tests under testGraalJS. The 3.0.0-SNAPSHOT build carries the
setter-based fix (enonic/lib-text-encoding#157).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PinKDCEKkM3QeGTnSYQ5zg

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

GraalJS Compatibility

2 participants