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

feat(console): reuse deno console's impl #3468

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

Conversation

ahaoboy
Copy link
Contributor

@ahaoboy ahaoboy commented Nov 15, 2023

This Pull Request reuses deno's console implementation with the following changes:

  1. Global function __boa_no_color controls whether the output contains color information.
  2. Global function __boa_print is used for outputting characters one by one.

The testing use a hacky solution by overriding __boa_print and __boa_no_color to store colorless output into the property __hack_print_output of a global object.

An odd problem occurs when attempting to output an array containing Chinese characters using console.log(['你', '好']). However, the code runs successfully if the join() function is used before the output: console.log(['你', '好'].join()). I don't know how to fix this...

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a8aad0b) 44.59% compared to head (5ff3d58) 46.14%.

Files Patch % Lines
boa_runtime/src/console/mod.rs 82.35% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3468      +/-   ##
==========================================
+ Coverage   44.59%   46.14%   +1.55%     
==========================================
  Files         487      487              
  Lines       50601    50402     -199     
==========================================
+ Hits        22563    23257     +694     
+ Misses      28038    27145     -893     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Nov 18, 2023

The error appears to be an issue with the RegExp, so this PR is ready to merge.

#3474

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Nov 20, 2023

The issue ridiculousfish/regress#74 was fixed, but no new version has been released. I validated it locally using the latest version.

@jedel1043
Copy link
Member

We discussed this in our triage and we think having to load a whole script when loading the console is a little bit funky. Maybe this could benefit from extracting the essentials of the display algorithms and reimplementing them using native Rust code.

@jedel1043 jedel1043 added waiting-on-author Waiting on PR changes from the author builtins PRs and Issues related to builtins/intrinsics labels Nov 30, 2023
@ahaoboy
Copy link
Contributor Author

ahaoboy commented Dec 6, 2023

We discussed this in our triage and we think having to load a whole script when loading the console is a little bit funky. Maybe this could benefit from extracting the essentials of the display algorithms and reimplementing them using native Rust code.

I haven't tested what impact introducing this huge JS file will have on size and performance. I'm also more inclined to rewrite the log function using Rust, but that will be a long-term process and beyond my capabilities.

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Jan 30, 2024

For anyone wanting to use deno's console implementation , they can try deno-console. Boa indeed should not introduce such complex logic; it just needs to support atomic types in JavaScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics waiting-on-author Waiting on PR changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants