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

No colours when run on Node.js #383

Closed
keynmol opened this issue Jan 21, 2023 · 14 comments
Closed

No colours when run on Node.js #383

keynmol opened this issue Jan 21, 2023 · 14 comments

Comments

@keynmol
Copy link
Contributor

keynmol commented Jan 21, 2023

Consider this file:

//> using lib "com.outr::scribe::3.10.6"

@main def hello =
  scribe.info("Howdy y'all")

When run via JVM, it's beautiful
image

When run through JS, it's bleak
image

To ensure that node.js supports ansi colour codes, I tried this
image

So it's merely an issue of detecting which JS platform the code runs on.

I would suggest using the hack outlined in detect-node package: https://github.com/iliakan/detect-node/blob/master/index.js#L2

Or may be there's a simpler approach in Scala.js

@keynmol
Copy link
Contributor Author

keynmol commented Jan 21, 2023

Also, as another (but much harder to reproduce) datapoint, my tooling captures STDERR from a node process which Scribe logs to, and I get this:
image

Do you know what those %c symbols are? I couldn't reproduce in a standalone node run.

@darkfrog26
Copy link
Contributor

The Scala.js support was originally specifically focused on browser output. Try this to see if it fixes your problems:

Logger.root.clearHandlers().withHandler(outputFormat = ANSIOutputFormat).replace()

The problem is this: https://github.com/outr/scribe/blob/master/core/js/src/main/scala/scribe/Platform.scala#L20

Which uses: https://github.com/outr/scribe/blob/master/core/js/src/main/scala/scribe/output/format/RichBrowserOutputFormat.scala that is specifically designed for rich output to the browser console. If we can add detection of browser vs Nodes.js, then we can differentiate which output style to use automatically.

Let me know if the code above fixes it and we can work toward a patch to make it automatic.

@keynmol
Copy link
Contributor Author

keynmol commented Jan 21, 2023

Let me know if the code above fixes it and we can work toward a patch to make it automatic.

This worked, thank you!

that is specifically designed for rich output to the browser console

nice, I had no idea %c existed and that it applies CSS

@darkfrog26
Copy link
Contributor

Now, we just have to come up with a way to differentiate between running in the browser and running in Node.js. Any ideas? :)

@keynmol
Copy link
Contributor Author

keynmol commented Jan 21, 2023

I would suggest using the hack outlined in detect-node package: https://github.com/iliakan/detect-node/blob/master/index.js#L2

I actually mentioned it in the original issue :)

basically if there's a global process object, then it's a Node.js environment (it seems)

Things actually get hairier because there's now at least Deno and Bun runtimes as well, but I think both are marginal in comparison to Node.js

@darkfrog26
Copy link
Contributor

Thanks, I'll take a look when I have a chance or feel free to submit a PR.

@darkfrog26
Copy link
Contributor

Is this a reasonable test to validate the availability of ANSI in Node.js?

sys.env.contains("TERM")

It's what I use on JVM and Native, but I'm not sure if the environment stuff all works the same within Node.js?

@armanbilge
Copy link

No, sys.env does not work in Scala.js, even on Node.js. You'd have to use process.env.

https://github.com/typelevel/cats-effect/blob/a7ca501029413ec1d62c0b6ea41893043852ea75/std/js/src/main/scala/cats/effect/std/EnvCompanionPlatform.scala

@darkfrog26
Copy link
Contributor

@armanbilge nice, thanks!

How's this look?

  private def processEnv: Dictionary[Any] = Try(js.Dynamic.global.process.env.asInstanceOf[js.Dictionary[Any]])
    .getOrElse(js.Dictionary.empty)

  override def env(key: String): Option[String] = processEnv.get(key).map(_.toString)

@armanbilge
Copy link

Seems fine :)

@darkfrog26
Copy link
Contributor

I've created this PR: #384

Feel free to make any suggested changes before I merge and release.

@darkfrog26
Copy link
Contributor

Release 3.10.7 includes the fix for this. @keynmol can you please verify this automatically works now before you close this ticket? Thanks!

@keynmol
Copy link
Contributor Author

keynmol commented Jan 23, 2023

That did it, thanks!

@keynmol keynmol closed this as completed Jan 23, 2023
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

No branches or pull requests

3 participants