Provide console API in shell. #1103

MaxisTekfield · 2021-11-28T07:34:51Z

As #1012 says, I've been trying to implement the console API in shell.

PR #1104 is prerequisite as p-bakker said. After #1104 is merged. I'll rebase this PR.

Basic usage:

js> console.info('obj: %o', {a: 123, b: "abc"})
INFO obj: {"a":123,"b":"abc"}
js> console.error("Error message.");
ERROR Error message.
js> console.count('counter1')
INFO counter1: 1
js> console.count('counter1')
INFO counter1: 2
js> console.count('counter1')
INFO counter1: 3
js> console.countReset('counter1')
js> console.count('counter1')
INFO counter1: 1
js> console.time()
js> console.timeLog()
INFO default: 3804.4751ms
js> console.timeLog()
INFO default: 7525.4994ms
js> console.timeEnd()
INFO default: 15393.5158ms
js> console.timeEnd()
WARN Timer 'default' does not exist.

Closes #1012

p-bakker · 2021-11-28T09:10:23Z

Great work!

As for the reformatting of Globals.java: we tend to do reformatting in a separate commit/PR, so formatting and actual changes are separated. Any chance you can create a separate PR for that?

As for the console implementation: seems like the implementation is tied directly to the Shell via the print method, from which I assume the console implementation cannot be used by embeddings that don't use the Shell?

MaxisTekfield · 2021-11-28T11:13:42Z

PR #1104 is created for reformatting Global.java.

I've thought about using console in embedding usage. I believe that is entirely possible.
But I am confusing which mechanism should be used to provide extension for implying console API in embedding usage. Java SPI or something else?

p-bakker · 2021-11-28T11:27:53Z

As for how to enable the console in an embedded Rhino: the embedder can can the init method themselves

I think the more important part is how to allow the embedder to control how an 'output' is being handled

For the Shell is being dealt with by calling the println method. In an embedding it might have to be handled by calling the appropriate method(s) of the used log library

So i think maybe define some interface and require to pass an instance of a class implementing that interface to the init method of the NativeConsole?

That way what's being done with the output is decoupled from the console implementation

p-bakker · 2021-11-28T12:05:46Z

Exactly what such interface should look like I'm not exactly sure: some implementations might want the raw data (for example an object passed as argument to console.log) while others would rather have the entire data as an already formatted string

And some might want to know the log level.

Btw: another option instead of an interface would be to provide a basic implementation based on System.out.println and just extend NativeConsole and override that implementation with whatever is desired

MaxisTekfield · 2021-11-29T07:25:50Z

How about the following solution.

First, when and where should I put the `NativeConsole` into scope?

I'm thinking these two ways:

Insert some code in org.mozilla.javascript.ScriptRuntime#initSafeStandardObjects - just like NativeJSON - everybody can use console by default.
Or (maybe not necessary), based on 1, add new feature switch to enable/disable the NativeConsole implementation. Default is disabled for compatibility reason. Someone may has already implemented his own console.

Second, how to provide interface to let applications embedding rhino-runtime to implement their own output?

I found org.mozilla.javascript.VMBridge#makeInstance to be a good example. I can try following class names in order to find the implementation to do outputs.

org.mozilla.javascript.console.ConsoleOutput_custom // applications can provide class with this name to implement their own outputs.
org.mozilla.javascript.tools.shell.ConsoleOutput // the output of shell.
org.mozilla.javascript.console.DefaultConsoleOutput // output by System.out and System.err.

p-bakker · 2021-11-29T07:52:32Z

First, when and where should I put the NativeConsole into scope?

I would say neither of the two options you mentioned. We can hadcode the console to be available in the Shell, but that's about it. For all other embeddings, it should be up to the embedder whether to include it or not and I think a feature flag is not really necesarry, an embedding has code already to initialize the embedding, like setting the language level etc. Adding a call there to initialize the console if they want is sufficient imho.

Second, how to provide interface to let applications embedding rhino-runtime to implement their own output?

Why not just NativeConsole.init(scope, sealed, new ConsoleImpl(){....} or new NativeConsole(scope, sealed) {...}

tuchida · 2021-11-29T08:05:04Z

Very nice PR!
Do you need to test? This is the specification, and this seems to be the test.

toolsrc/org/mozilla/javascript/tools/shell/NativeConsole.java

MaxisTekfield · 2021-11-29T13:54:00Z

I have made some changes in order to make printer customizable in embedding usage.

Do you need to test?

Yes, I need. I'll write them later.

gbrail · 2021-11-29T20:26:46Z

I think it'd be great to have "console"! But I do think that we should only have it in Global (which makes it part of the shell) and:

Make it easy for any embedder to add it to any script, but not by default, even if they don't use "Global."
Make the choice of output location pluggable by supplying an OutputStream.

tonygermano · 2021-11-29T20:50:28Z

As discussed in #1012 I think the plugable printer needs to take an Object[] (or List<Object>.)

If the first argument is a string, then NativeConsole can perform the substitutions, but any remaining arguments that are not used in the substitution or all of the arguments if the first is not a string should be passed to the printer to handle as appropriate.

Some printers may want to display debug level detail (possibly in a GUI like the Chrome and Firefox consoles) for nested objects rather than just performing a toString operation on them.

A Context object should also be passed to facilitate accessing the (nested) object properties.

That doesn't preclude a function that would return an implementation that would do that basic conversion.

NativeConsole.ConsolePrinter createToStringConsolePrinter(OutputStream trace, OutputStream debug, OutputStream info, OutputStream warn, OutputStream error)

p-bakker · 2021-11-29T21:21:48Z

I think the plugable printer needs to take an Object[] (or List.)

Agree, also from the perspective of stringifying the whole thing being unneeded (and potentially expensive) if for example console.debug(...) is called, while the underlying output/log facitity might be configured to ignore everything lower than log level INFO

MaxisTekfield · 2021-12-01T07:10:30Z

I've changed args of print to Object[].

src/org/mozilla/javascript/NativeConsole.java

gbrail · 2022-01-27T01:38:00Z

Thanks!

Can you follow the comments above and get rid of the now-obsolete "idswitch" stuff? You can look at any of the existing classes for an example.

Also, it'd be great to take a look at the man page (the file is rhino/man.1) and see if we can update it to mention that the "console" object is now supported.

p-bakker · 2022-01-29T17:10:55Z

src/org/mozilla/javascript/NativeConsole.java

+                return "Console";
+
+            case Id_trace:
+                printer.print(cx, scope, Level.TRACE, args);


Shouldn't console.trace() automatically send the current stacktrace as the first entry in the args object[] to the printer?

How about the latest code which passes ScriptStackElement[] stack after Object[] args.

p-bakker · 2022-02-15T07:53:50Z

@MaxisTekfield any chance you could followup on the previous two comments on this PR?

would be nice to get this ready to be merged

MaxisTekfield · 2022-02-16T08:55:18Z

@MaxisTekfield any chance you could followup on the previous two comments on this PR?

Got it. These takes more time to make changes. I'll make them done in next few days.

tuchida · 2022-03-01T11:20:19Z

src/org/mozilla/javascript/NativeConsole.java

+
+                case "%d":
+                case "%i":
+                    double number = ScriptRuntime.toNumber(args, argIndex);


BigInt will cause an error.

console.log('%d', 10n) // js: uncaught JavaScript runtime exception: TypeError: Cannot convert BigInt to a number

The specifications are as follows.
https://console.spec.whatwg.org/#formatting-specifiers

%d or %i | Element which substitutes is converted to an integer | %parseInt%(element, 10)

The implementation are as follows.

V8 (Node.js v14.17.0)

console.log('%d', 10n) // 10n console.log('%f', 10n) // NaN

SpiderMonkey (Firefox/97.0) (ref. https://bugzilla.mozilla.org/show_bug.cgi?id=1753682)

console.log('%d', 10n) // undefined console.log('%f', 10n) // undefined

Format specifier for BigInt whatwg/console#148

I didn't know what was right at the time of %f.

console.log('%d', Symbol.iterator) // js: uncaught JavaScript runtime exception: TypeError: The object is not a number

According to the specification, it seems that Symbol should be converted to NaN.
https://console.spec.whatwg.org/#formatter

If Type(current) is Symbol, let converted be NaN

Even %o or %O will cause an error.

console.log('%o', { a: 0n }) // js: uncaught JavaScript runtime exception: TypeError: Do not know how to serialize a BigInt

I got a syntax error. How to represent a BigInt in rhino shell?

js> print(10n); js: "<stdin>", line 10: missing ) after argument list js: print(10n); js: .........^ js: "<stdin>", line 10: Compilation produced 1 syntax errors. js> console.log("%d", 10n); js: "<stdin>", line 12: missing ) after argument list js: console.log("%d", 10n); js: .....................^ js: "<stdin>", line 12: Compilation produced 1 syntax errors. js>

Could you add -version 200.

$ java -jar buildGradle/libs/rhino-1.7.15-SNAPSHOT.jar -version 200

Even %o or %O will cause an error.

console.log('%o', { a: 0n }) // js: uncaught JavaScript runtime exception: TypeError: Do not know how to serialize a BigInt

I think this is not the problem of console. This TypeError is thrown at org.mozilla.javascript.NativeJSON#str

if (value instanceof Number) { if (value instanceof BigInteger) { throw ScriptRuntime.typeErrorById("msg.json.cant.serialize", "BigInt"); } // ... }

This problem also occurs when using JSON.stringify

js> JSON.stringify({a: 10n}); js: uncaught JavaScript runtime exception: TypeError: Do not know how to serialize a BigInt at (unknown):9

Should we modify org.mozilla.javascript.NativeJSON#str in order to output BigInt when using console and JSON.stringify APIs? Maybe starting another issue is better.

Yes, I do. It is a specification of ECMA262 that JSON.stringify will error on BigInt.
https://262.ecma-international.org/12.0/#sec-serializejsonproperty

If Type(value) is BigInt, throw a TypeError exception.

Even in Node.js, using %j will result in an error.

console.log('%j', 10n) // Uncaught TypeError: Do not know how to serialize a BigInt console.log('%o', 10n) // 10n

Other than that, JSON.stringify will cause an error with circular references.

var o = {}; o.o = o; JSON.stringify(o); // js: uncaught JavaScript runtime exception: TypeError: Cyclic org.mozilla.javascript.NativeObject value not allowed.

If you definitely don't want to generate errors, you should use a method other than JSON.stringify to display the data. However, for now, I think it is enough to catch the exception and fall back to ScriptRuntime.toString.

src/org/mozilla/javascript/NativeConsole.java

tuchida · 2022-03-03T14:47:58Z

src/org/mozilla/javascript/NativeConsole.java

+        printer.print(cx, scope, level, null, new String[] {msg});
+    }
+
+    public static String format(Context cx, Scriptable scope, Object[] args) {


I have had some corrections made, could you write tests for them?

13192a2 Thanks!

It's hard to cover all those specifications, especially those who have different behaviors in different runtime engines.

gbrail · 2022-03-11T23:48:36Z

Thanks for all the work on this -- I hope that we can keep making it more complete. I'm going to ask for a few more things, but I think that sooner or later we can get this working...

First, can you move NativeConsole to the org.mozilla.javascript.tools package in the "toolsrc" tree? The reason is that everything like this, that prints to standard output for example, should be in that separate package so that it doesn't clutter up the main "src" tree which IMO should just be the basic JavaScript engine. The rest of the shell stuff works this way.

Second, can we get at least some basic test coverage of the basic functions? It looks like some of it is there (thanks!) and some will be hard to test, like the "time" function, so there at least a test to prove it doesn't bomb out would be enough.

Finally, it seems like "console" could just be a garden-variety object (created using cx.newObject for example) and each property could be an instance of LambdaFunction. That might avoid all of the complicated IdScriptableObject stuff. But I can look at that later, it's just an idea to simplify things.

p-bakker · 2022-03-12T05:50:13Z

First, can you move NativeConsole to the org.mozilla.javascript.tools package in the "toolsrc" tree?

Am not sure if this is a good idea: the console is (imho) something embedders and script-engine users might also want to use. Making them require to use the bundle that includes all the tools would not be so great I think

MaxisTekfield · 2022-03-15T02:33:50Z

First, can you move NativeConsole to the org.mozilla.javascript.tools package in the "toolsrc" tree?

Am not sure if this is a good idea: the console is (imho) something embedders and script-engine users might also want to use. Making them require to use the bundle that includes all the tools would not be so great I think

Emm, I don't know what should I do.
I'd like to keep console in main src because I think it will be helpful for all those users - embedders and shell users.

tuchida · 2022-03-15T05:14:08Z

Deno and Node.js seem to be able to use console without import.
https://doc.deno.land/deno/stable/~/console
https://nodejs.org/dist/latest-v17.x/docs/api/console.html

gbrail · 2022-03-17T00:10:17Z

I would be concerned about this being in the main package if it did things like open files or execute processes like our "Global" object does. Since it doesn't, and if others disagree, then I'm fine with having it in the main package where it is in this PR.

So then the only remaining thing is to see if we can at least have a sanity test of each function in the API.

Thank you for all the work on this!

MaxisTekfield · 2022-03-23T02:55:39Z

More tests added.

gbrail · 2022-04-01T21:30:04Z

Thanks for all the work. Since this requires a merge and has many commits, I'm going to close this PR manually and merge the code into master.

p-bakker · 2022-04-04T09:35:28Z

Tnx @MaxisTekfield for all your work!

MaxisTekfield force-pushed the console-api branch from d0ac17b to 6f29fa5 Compare November 28, 2021 07:38

MaxisTekfield mentioned this pull request Nov 28, 2021

Reformat Global.java for future modifications. #1104

Merged

p-bakker reviewed Nov 29, 2021

View reviewed changes

toolsrc/org/mozilla/javascript/tools/shell/NativeConsole.java Outdated Show resolved Hide resolved

MaxisTekfield added 3 commits December 11, 2021 23:17

Provide console API in shell.

8f7acd0

Make console API available in embedding usage.

8452ed6

Change args of ConsolePrinter to Object[].

bd692bf

MaxisTekfield force-pushed the console-api branch from 8905c3f to bd692bf Compare December 11, 2021 15:21

tuchida reviewed Jan 25, 2022

View reviewed changes

src/org/mozilla/javascript/NativeConsole.java Outdated Show resolved Hide resolved

p-bakker reviewed Jan 29, 2022

View reviewed changes

MaxisTekfield force-pushed the console-api branch from 3042569 to ae1b6a9 Compare February 27, 2022 11:59

Use String in findPrototypeId.

ae1b6a9

tuchida reviewed Mar 1, 2022

View reviewed changes

MaxisTekfield force-pushed the console-api branch from 9c4eb6c to 178d18c Compare March 1, 2022 13:54

MaxisTekfield added 2 commits March 1, 2022 22:54

Add console API into man page.

ee7b110

Pass stack to printer when running console.trace().

868d043

MaxisTekfield force-pushed the console-api branch from 178d18c to 868d043 Compare March 1, 2022 15:10

Process BigInt arguments in console API.

3e8d849

tuchida reviewed Mar 2, 2022

View reviewed changes

src/org/mozilla/javascript/NativeConsole.java Outdated Show resolved Hide resolved

Convert to long when using %d and %i.

2d17de4

tuchida reviewed Mar 3, 2022

View reviewed changes

Add tests for NativeConsole.

13192a2

Add more tests.

606e2c0

gbrail closed this Apr 1, 2022

p-bakker modified the milestone: Release 1.7.15 Apr 4, 2022

Provide console API in shell. #1103

Provide console API in shell. #1103

Conversation

MaxisTekfield commented Nov 28, 2021 • edited by p-bakker Loading

p-bakker commented Nov 28, 2021

MaxisTekfield commented Nov 28, 2021

p-bakker commented Nov 28, 2021

p-bakker commented Nov 28, 2021

MaxisTekfield commented Nov 29, 2021

First, when and where should I put the NativeConsole into scope?

Second, how to provide interface to let applications embedding rhino-runtime to implement their own output?

p-bakker commented Nov 29, 2021

tuchida commented Nov 29, 2021

MaxisTekfield commented Nov 29, 2021 • edited Loading

gbrail commented Nov 29, 2021

tonygermano commented Nov 29, 2021

p-bakker commented Nov 29, 2021 • edited Loading

MaxisTekfield commented Dec 1, 2021 • edited Loading

gbrail commented Jan 27, 2022

Choose a reason for hiding this comment

MaxisTekfield Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

p-bakker commented Feb 15, 2022 • edited Loading

MaxisTekfield commented Feb 16, 2022

tuchida Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxisTekfield Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxisTekfield Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented Mar 11, 2022

p-bakker commented Mar 12, 2022

MaxisTekfield commented Mar 15, 2022

tuchida commented Mar 15, 2022

gbrail commented Mar 17, 2022

MaxisTekfield commented Mar 23, 2022

gbrail commented Apr 1, 2022

p-bakker commented Apr 4, 2022

MaxisTekfield commented Nov 28, 2021 •

edited by p-bakker

Loading

First, when and where should I put the `NativeConsole` into scope?

MaxisTekfield commented Nov 29, 2021 •

edited

Loading

p-bakker commented Nov 29, 2021 •

edited

Loading

MaxisTekfield commented Dec 1, 2021 •

edited

Loading

MaxisTekfield Mar 2, 2022 •

edited

Loading

p-bakker commented Feb 15, 2022 •

edited

Loading

tuchida Mar 1, 2022 •

edited

Loading

MaxisTekfield Mar 2, 2022 •

edited

Loading

MaxisTekfield Mar 2, 2022 •

edited

Loading