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

JSBridge-Core-ct.11: Fix superclass of JSException #155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LinqLover
Copy link
Contributor

One should (always) never subclass from Exception itself, but most of the time from Error or Notification or one of its subclasses. Exception is an abstract class, and the previous implementation led to SubclassResponsibility errors from Exception>>defaultAction when signaling a JSException. Since JavaScript errors do not have Smalltalk''s coroutine capabilities, subclassing from Error seems a perfect match in this case.

One should (always) never subclass from Exception itself, but most of the time from Error or Notification or one of its subclasses. Exception is an abstract class, and the previous implementation led to SubclassResponsibility errors from Exception>>defaultAction when signaling a JSException. Since JavaScript errors do not have Smalltalk''s coroutine capabilities, subclassing from Error seems a perfect match in this case.
@codefrau
Copy link
Owner

Interesting, thanks! Could you please also update JSBridge.st which is used by pre-Monticello images?

And while you're at it, I think we should update JSObjectProxy>>await: so it works in old images without exception handling (e.g. Dan's mini.image with our default JSBridge demo, which BTW is why the primitives are declared the way they are). I have not tested it but something like this, maybe?

...
isError ifTrue: [
	JSException superclass
		ifNil: [self error: result]
		ifNotNil: [JSException error: result]].

@marceltaeumel
Copy link

In Squeak/Smalltalk, one can implement dynamic scope through Exception. One example is CurrentReadOnlySourceFiles.

One should (always) never subclass from Exception itself [...]

...if one means to model exceptions in a somewhat traditional sense. Yes. In Squeak, there are "notifications in disguise" (i.e., resumable exceptions) such as WebAuthRequired and MCNoChangesException. There are also "errors in disguise" (i.e., non-resumable exceptions) such as TestFailure. And some special things such as Halt and UnhandledError, the former almost an error but resumable. So much fun. :-)

@LinqLover
Copy link
Contributor Author

@codefrau Sorry for the delay!

Yes, your fix works in the mini image. I prefer to add this to the beginning of JSException>>error::

self class superclass ifNil: [^ nil error: error asString].

Other than that, I think I found a few more issues:

SqueakJS does not support custom error values

The ECMA specs do not require values that are passed to throw1 or Promise.reject2 to be Error instances, but allows arbitrary values such as 42 or "hi" as well. E.g., throw 42; or new Promise((resolve, reject) => reject(42)); are valid though not recommended. How can we fix that?

throw

Example:

JS eval: 'throw 42'. -->Error: undefined "expected: Error: 42"

Possible alternative fixes:

  1. Fix error decoding in the VM plugin: In vm.plugins.javascript.js, use err instanceof Error ? err.message : String(err) to assure a meaningful error message.
  2. Delegate error decoding from VM to image: In vm.plugins.javascript.js, collect and return true throw values (e.g., Exception objects or other values). In JSBridge, wrap the result of primGetError into a JSException and signal that rather a generic error:. In addition, support non-Errors in JSException (see below).

Discussion:

  • Pro 1:
    • Smallest change.
    • Maintains backward compatibility of the plugin interface. (If compatibility matters, we could also offer a new primitive in addition to the old one.)
  • Pro 2:
    • Stops omitting information and allows for introspection of any thrown values in the image.
    • Consistent use of JSException: Signaling specific subclasses of Error is commonly better style in Squeak and simplifies nunaced exception handling.

Promise.reject()

Example:

JS eval: 'new Promise((resolve, reject) => reject(42))'. -->Error: undefined "expected: Error: 42"

Possible fixes:

  1. Decode errors in the image: In JSException, add something like ((JS eval: '(e) => e instanceof Error ? e.message : String(e)') call: undefined value: error) asString (all the indirection because we cannot access instanceof through the plugin and the equivalent of myString.property in JSBridge does not answer undefined but signals an error)).
  2. Decode errors in the VM and deliver them separately into the image: As suggested above, maybe add a new primitveGetErrorObject to the existing one and use both when constructing a JSException object.

Discussion:

  • Pro 1: Keeps plugin interface minimal.
  • Pro 2: Avoids handshakes between VM and image (I assume faster). In line with the backward-compatible proposal from above.

tl;dr my preferred solution would be to add a new primitiveGetErrorObject to the plugin to provide both the raw error value/string/whatever and a string/message version separately to the image; use it in JSException, and reuse the JSException route in JSObjectProxy.

Default SqueakJS GUI does not support promises

Set up a fresh trunk image, installed JSBridge-Core, and opened it in https://squeak.js.org/run. Example:

JS await: (JS eval: 'new Promise(resolve => resolve(42))')

Similarly, JS eval: 'await 42' fails due to lack of asynchronous context.

Times out forever. /cc'ing @tom95 & @leogeier who made a similar observation.


Sorry for the wall of text. Looking forward to your opinions! Yes, I will patch JSBridge.st as well.

Footnotes

  1. https://tc39.es/ecma262/multipage/ecmascript-language-statements-and-declarations.html#sec-throw-statement

  2. https://tc39.es/ecma262/multipage/control-abstraction-objects.html#sec-promise.reject

@codefrau
Copy link
Owner

I like your analysis! I agree we should wrap thrown objects in JSException rather than convert them to strings. I just added a primitiveGetErrorObject to the VM (see ab9b818).

For historical context: JSException got added by Bill Burdick (WRB) to my initial JSBridge implementation where I only used self error: 'string'. However, we did not change over everything to exceptions. Now is a good time to do so.

Re: JSException>>error: for pre-Exception images – your suggestion sounds reasonable. It would be even nicer if we could send error: not to nil but to the JSObjectProxy that encountered the error, for a nicer debugging experience. Something like JSException error: xyz in: self and store both the proxy and the error (or whatever was thrown) in the exception.

Re: "SqueakJS GUI does not support promises" – works on my machine 😉
In the mini image, JS await: (JS eval: 'new Promise(resolve => resolve(42))') works as expected, answering 42. There must be something different in the trunk image (I haven't tried that).

I'm not worried about JS eval: 'await 42' failing because that is the case for a plain JS eval('await 42') too. It's precisely why we have await:.

@LinqLover
Copy link
Contributor Author

@codefrau Sorry for not replying earlier! While trying to debug JS await:, I stumbled upon some other primitive issues in SqueakJS and tried to fix them first (see my recent trunk versions for Kernel/KernelTests and my PRs in this repo). I think most of them should be fixed now, and I'm looking forward to your reviews 🙂, but half a week is gone mainly for that and I guess I should return to my actual work before I continue exploring promises in SqueakJS ... so I will have to put this on ice for now and return later, hopefully soon!

Just some very brief replies below:

I like your analysis! I agree we should wrap thrown objects in JSException rather than convert them to strings. I just added a primitiveGetErrorObject to the VM (see ab9b818).

Thanks! Looking forward to integrate them on the image side soon if you do not preempt me. :-)

For historical context: JSException got added by Bill Burdick (WRB) to my initial JSBridge implementation where I only used self error: 'string'. However, we did not change over everything to exceptions. Now is a good time to do so.

Re: JSException>>error: for pre-Exception images – your suggestion sounds reasonable. It would be even nicer if we could send error: not to nil but to the JSObjectProxy that encountered the error, for a nicer debugging experience. Something like JSException error: xyz in: self and store both the proxy and the error (or whatever was thrown) in the exception.

I like that!

Re: "SqueakJS GUI does not support promises" – works on my machine 😉 In the mini image, JS await: (JS eval: 'new Promise(resolve => resolve(42))') works as expected, answering 42. There must be something different in the trunk image (I haven't tried that).

It works on the demo page for me too, but not on https://squeak.js.org/run. Do they share the same GUI code?

I'm not worried about JS eval: 'await 42' failing because that is the case for a plain JS eval('await 42') too. It's precisely why we have await:.

@codefrau
Copy link
Owner

It works on the demo page for me too, but not on https://squeak.js.org/run. Do they share the same GUI code?

Yes, the GUI is created by createSqueakDisplay which is shared by all pages. Only when running on Lively there is different GUI code.

After running the demo, you should find "squeakjs.image" on the /run page. Clicking that works just fine for me.

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.

None yet

3 participants