Bug JavaScript bugs

philmasterplus

Active member
Confirmed that as of r20539, JS-based relay scripts are working...almost. Chrome shows the returned HTML source verbatim.

incorrect-content-type.png

Cursory look at devtools indicates that KoLmafia is returning Content-Type: text/javascript in the HTTP response headers. Special-casing this would be appropriate, since we don't want to break existing cases where script authors serve JS files from within the relay/ directory. Fixed in r20550

Two minor issues I've seen so far.

Proxy record fields
Proxy record fields that return other proxy records don't seem to be wrapped properly as JS objects, even in r20539. For example, the following code:
JavaScript:
const { print } = require("kolmafia");
print(typeof Familiar.get("Angry Goat").hatchling);

...will throw with:
Code:
JavaScript evaluator exception: Invalid JavaScript value of type net.sourceforge.kolmafia.textui.parsetree.Value (file:/C:/Users/Phil/Documents/KoL/scripts/proxy-record-test.js#2)
at file:/C:/Users/Phil/Documents/KoL/scripts/proxy-record-test.js:2

Also, these "nested" proxy records cannot be compared with regular objects. When tested in the gCLI:
Code:
> js Familiar.get("Angry Goat").hatchling === Item.get("goat")
Returned: false

The workaround for this is to manually convert the field to string, then call the get() function:
Code:
> js typeof Item.get(String(Familiar.get("Angry Goat").hatchling)
Returned: object
> js Item.get(String(Familiar.get("Angry Goat").hatchling)) === Item.get("goat")
Returned: true

Importing toString
Attempting to import toString from kolmafia like this...'
JavaScript:
const { toString } = require("kolmafia");

...fails with: JavaScript error: TypeError: redeclaration of var toString.

It can be trivially avoided by assigning the namespace itself, or by renaming the function:
JavaScript:
const Lib = require("kolmafia"); // Use Lib.toString()
const { toString: formatString } = require("kolmafia");

Since this one is so easy to work around, this is more of a note rather than a request to fix.
 
Last edited:

philmasterplus

Active member
I found a fix for the content-type problem with relay JS scripts (mentioned above). A file extension check early in the if-else chain made the relevant code path unreachable.

Tested on r20544, confirmed that accessing paths that look like my_script.js?relay=true results in a content-type of text/html.

Edit: This has been committed in r20550
 

Attachments

  • philmasterplus-js-relay-content-type-fix.patch
    1.2 KB · Views: 4
Last edited:

ikzann

Member
Thanks for finding that fix. I'll take a look at the value conversion issues you mentioned.

A fix to another problem with importing ASH scripts that overload functions:
 

Attachments

  • ash-overloading.patch
    1.3 KB · Views: 3

fronobulax

Developer
Staff member
Can someone merge those two patches?
Vocabulary issue? Do you mean commit to svn the patch in post 43 and then commit the patch in 45 as two sequential operations? If so I am reviewing now.

If you mean combining the patches and some how resolving conflicts before committing, I'm not going to do that. I'll commit one and if the second errors I will ask the author to resolve the issue.

Stand by.
 

gausie

D̰͕̝͚̤̥̙̐̇̑͗̒e͍͔͎͈͔ͥ̉̔̅́̈l̠̪̜͓̲ͧ̍̈́͛v̻̾ͤe͗̃ͥ̐̊ͬp̔͒ͪ
Staff member
Vocabulary issue? Do you mean commit to svn the patch in post 43 and then commit the patch in 45 as two sequential operations? If so I am reviewing now.

If you mean combining the patches and some how resolving conflicts before committing, I'm not going to do that. I'll commit one and if the second errors I will ask the author to resolve the issue.

Stand by.
Frono come on this is ridiculous.
 

fronobulax

Developer
Staff member
Frono come on this is ridiculous.

You are entitled to your opinion. In the svn context there is a "merge" command that is distinct from a "commit". In casual usage in an svn context, "merge" means reconciling conflicts between competing edits on the same files. So I wanted to find out if that reconciliation was expected, or not.
 

ikzann

Member
Here's a fix for the value conversion issue that philmasterplus was seeing, test with js Monster.get(33).phylum === Phylum.get('undead')
 

Attachments

  • proxyfix.patch
    2.6 KB · Views: 2

MCroft

Developer
Staff member
Error in js Item.get("sword")

Unexpected error, debug log printed.
class net.sourceforge.kolmafia.textui.ScriptException: Bad item value: sword
net.sourceforge.kolmafia.textui.ScriptException: Bad item value: sword
Did we decide that a debug log message was correct for <thing> not found? I feel like we talked about it a few places, but I'm not sure what the resolution was.

It feels like throwing in this case is too much. It doesn't indicate a code error in Mafia that needs a stack trace.
Edit: What I'd like is that the script exception is caught in the gCLI and shows a mafia error (red sidebar). What happens is that it throws an error to the debug log and shows a green sidebar.

Code:
> js Monster.get("zombie")
Unexpected error, debug log printed.
Returned: null

class net.sourceforge.kolmafia.textui.ScriptException: Bad monster value: zombie
net.sourceforge.kolmafia.textui.ScriptException: Bad monster value: zombie
...
 

philmasterplus

Active member
Ideally it should throw a TypeError exception inside the JavaScript runtime so the script author can handle it. IIRC it simply terminates the runtime.
 
as of r20567, using setProperty() to set a property to a numerical value will set that value as a float rather than an integer, which is problematic; the particular case I'm talking about here was setProperty('choiceAdventure1387', 1) setting the choice adventure to 1.0, which meant I couldn't kill my mother.

For now, I've changed the line to setProperty('choiceAdventure1387', '1'), which seems to be working, but strikes me as a bad and inelegant solution. At Ezandora's advisement, I'm posting the issue here. A sensible solution might be to default to making numbers without fractional values into ints as they get assigned to preferences.

Thanks!
 

ikzann

Member
as of r20567, using setProperty() to set a property to a numerical value will set that value as a float rather than an integer, which is problematic; the particular case I'm talking about here was setProperty('choiceAdventure1387', 1) setting the choice adventure to 1.0, which meant I couldn't kill my mother.

For now, I've changed the line to setProperty('choiceAdventure1387', '1'), which seems to be working, but strikes me as a bad and inelegant solution. At Ezandora's advisement, I'm posting the issue here. A sensible solution might be to default to making numbers without fractional values into ints as they get assigned to preferences.

Thanks!
This is unfortunately a tricky limitation to work around in the JS-to-Java-to-ASH conversion chain and any fix is going to be a little bit of a hack. I'd recommend using TypeScript to avoid typing issues like this. I will think about how to fix this particular issue.
 

ikzann

Member
Ideally it should throw a TypeError exception inside the JavaScript runtime so the script author can handle it. IIRC it simply terminates the runtime.
We should fix the ultimate exception not being caught, but these are currently catchable in JS:
Code:
> jsq Item.get("sword")

Internal    exception: Wrapped net.sourceforge.kolmafia.textui.ScriptException: Bad    item value: sword (command line#1)
at command line:1
Unexpected    error, debug log printed.

> jsq try {    Item.get('sword'); } catch(e) { print(e.toString()); }

JavaException:    net.sourceforge.kolmafia.textui.ScriptException: Bad item value: sword
 

fronobulax

Developer
Staff member
Tour Guide, available from https://github.com/cdrock/TourGuide/branches/Release installs both relay_TourGuide.ash and relay_TourGuide.js in /relay.

This results in two entries labelled TourGuide in the relay browser run script dropdown. While the author could have made different choices this raises the question of how to handle similarly named scripts? Since an ash script and a js script are two different things, I think there is a feature request to distinguish between the two types in menus.

And, when I run relay_TourGuide.jsI I get
JavaScript evaluator exception: syntax error (file:/D:/Dropbox/dist/relay/relay_TourGuide.js#555)
and at this point I cannot tell whether that is a scripting error or an implementation error so am reporting it here.
 

philmasterplus

Active member
It wouldn't be difficult for Tour Guide's author to rename their script. I hope we don't end up with odd protocols like "relay override scripts must have a .rjs extension".
 

fronobulax

Developer
Staff member
It wouldn't be difficult for Tour Guide's author to rename their script. I hope we don't end up with odd protocols like "relay override scripts must have a .rjs extension".

My first thought was to clutter the various menus that have script names, to have the extension included in the GUI display.
 
Top