Skip to content

Conversation

@WardBrian
Copy link
Member

Submission Checklist

  • Run unit tests
  • Documentation
    • If a user-facing facing change was made, the documentation PR is here:
    • OR, no user-facing changes were made

Release notes

stanc.js can request ANSI escape code styling in errors and warnings by passing color-output as an argument. These are supported in most browser consoles.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

@WardBrian WardBrian requested a review from nhuurre November 7, 2025 15:37
Comment on lines 32 to 38
, (* NB: This should really be [Js.Unsafe.Inject (Js.array ...)] as above.
The fact that it is not is a historical mistake, and leads to the
resulting list in JS having a first entry of '0', the index.
Unfortunately, many tools have been built assuming the error message is
actually in the second entry, so we're stuck with this bad behavior.
*)
Js.Unsafe.inject (Array.map ~f:Js.string [| e |]) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we have been using Unsafe.inject incorrectly? I think it would be better to write something like

Js.Unsafe.inject (Js.array (Array.map ~f:Js.string [| "0" ; e |]) )

Or maybe just duplicate the error message?

let e = Js.string e in Js.Unsafe.inject (Js.array [| e ; e |])

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, more or less since the beginning. I only noticed this when building Stan Playground, and only spotted the fix while doing this change, but it's too late to really do much I think. Your suggestion of hard-coding the '0' seems reasonable to me given the choices.

Copy link
Member Author

Choose a reason for hiding this comment

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

To elaborate a bit more, the 0 came from the data representation used in jsoo:

array block with tag 0 (e.g. [|1;2|] => [0,1,2])

The missing Js.array call gets compiled down to caml_js_from_array, which is a 1-liner that returns .slice(1) of its argument

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does Js.Unsafe.inject differ from Js.Unsafe.coerce? The latter seems tiny bit more more type-safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're exactly the same runtime code (both get compiled away to nothing), the difference seems to be that coerce requires the input be a Js.t already, which would have caught this.

I'll switch to using that, nice suggestion

@WardBrian WardBrian merged commit a58dba6 into master Nov 10, 2025
3 checks passed
@WardBrian WardBrian deleted the stancjs-request-colors branch November 10, 2025 16:17
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.

3 participants