Skip to content

feat(value): add conversion and UTF-16 bindings#749

Merged
buke merged 2 commits into
mainfrom
feat/value-pr3-conversions-utf16
May 22, 2026
Merged

feat(value): add conversion and UTF-16 bindings#749
buke merged 2 commits into
mainfrom
feat/value-pr3-conversions-utf16

Conversation

@buke
Copy link
Copy Markdown
Owner

@buke buke commented May 22, 2026

  • add native quickjs-ng wrappers for number, index, bigint, property-key, and UTF-16 conversion APIs
  • add Context.NewStringUTF16 and focused regression coverage for success and error paths
  • keep full package coverage at 100.0%

- add native quickjs-ng wrappers for number, index, bigint, property-key, and UTF-16 conversion APIs
- add Context.NewStringUTF16 and focused regression coverage for success and error paths
- keep full package coverage at 100.0%
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds the NewStringUTF16 function to the Context type and introduces several conversion methods to the Value type, including ToNumber, ToIndex, ToBigInt64, ToBigUint64, ToInt64Ext, ToPropertyKey, and ToStringUTF16. The review feedback recommends using unsafe.SliceData for more idiomatic pointer acquisition and suggests optimizing exception handling in the ToNumber and ToPropertyKey methods to avoid unnecessary heap allocations when operations fail.

Comment thread context.go
Comment thread value.go
Comment thread value.go
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (1dbcb3c) to head (9155e80).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #749   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         3766      3831   +65     
=========================================
+ Hits          3766      3831   +65     
Files with missing lines Coverage Δ
context.go 100.00% <100.00%> (ø)
value.go 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dbcb3c...9155e80. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@buke
Copy link
Copy Markdown
Owner Author

buke commented May 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several new conversion methods to the Value and Context types, including NewStringUTF16, ToNumber, ToIndex, and various BigInt/Int64 conversions, along with comprehensive unit tests. The review feedback highlights a critical issue in the error handling logic across all these new methods: if a JavaScript exception is thrown that is not an Error object (e.g., a primitive), v.ctx.Exception() returns nil, causing the Go functions to return a nil error despite the failure. It is recommended to implement a fallback error to ensure robust error reporting.

Comment thread value.go
Comment thread value.go
Comment thread value.go
Comment thread value.go
Comment thread value.go
Comment thread value.go
Comment thread value.go
- add a centralized fallback error when QuickJS throws a non-Error exception value
- cover primitive throw paths in Context.Exception and the PR 3 conversion APIs
- keep full package coverage at 100.0%
@buke
Copy link
Copy Markdown
Owner Author

buke commented May 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several new features and improvements to the QuickJS wrapper, including a NewStringUTF16 method for creating strings from UTF-16 code units and a suite of conversion methods for the Value type, such as ToNumber, ToIndex, ToBigInt64, and ToStringUTF16. Additionally, exception handling in the Context has been refactored to include a private exceptionError helper. The review feedback suggests enhancing the error message for non-Error JavaScript exceptions by including the actual thrown value to aid debugging, along with corresponding updates to the test expectations in context_test.go and value_test.go.

Comment thread context.go
Comment thread context_test.go
Comment thread value_test.go
@buke
Copy link
Copy Markdown
Owner Author

buke commented May 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several new conversion methods to the Value type, including ToNumber, ToIndex, and ToStringUTF16, along with a new NewStringUTF16 method for the Context type. It also refactors exception handling to better capture JavaScript errors. Feedback suggests using unsafe.SliceData for more idiomatic slice pointer handling and improving error messages by using the string representation of thrown values instead of a generic fallback string.

Comment thread context.go
Comment thread context.go
@buke
Copy link
Copy Markdown
Owner Author

buke commented May 22, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several new conversion methods to the Value type—including ToNumber, ToIndex, and ToStringUTF16—and adds NewStringUTF16 to the Context type. Review feedback focuses on strengthening error handling to ensure that failures in the underlying C API always return an error, even if a specific JavaScript exception cannot be retrieved. Additionally, the reviewer suggested enhancing the exceptionError helper by including the string representation of thrown primitive values in error messages to improve debuggability.

Comment thread value.go
Comment thread value.go
Comment thread value.go
Comment thread context.go
@buke buke merged commit 1d5c2cf into main May 22, 2026
8 checks passed
@buke buke deleted the feat/value-pr3-conversions-utf16 branch May 22, 2026 13:23
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.

1 participant