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

breaking: Remove the FromArgs impl on T: TryFromJs and add cx.arg() and cx.arg_opt(). #1096

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Mar 24, 2025

A blanket impl was added to avoid duplicate methods or a confusing let (n,) = cx.args()?; syntax for unary functions. However, this does not work well with the #[neon::export] macro where functions with zero arguments end up unnecessarily deserializing undefined.

Removing the blanket impl and replacing it with a specialized version for () allows the proc macros to be more efficient without unneeded complexity.

In order to make these changes more ergonomic, I added Context::arg and Context::arg_opt for unary functions.

Attention: This is a breaking change, but the API has not landed in a stable version yet.

In the future, we could unify these APIs if Rust ships specialization.

…g()` and `cx.arg_opt()`.

A blanket impl was added to avoid duplicate methods or a confusing `let (n,) = cx.args()?;` syntax for unary functions. However, this does not work well with the `#[neon::export]` macro where functions with zero arguments end up unnecessarily deserializing `undefined`.
Copy link

codecov bot commented Mar 24, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.42%. Comparing base (7f08b76) to head (ac8603c).

Files with missing lines Patch % Lines
crates/neon/src/types_impl/extract/mod.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1096   +/-   ##
=======================================
  Coverage   83.42%   83.42%           
=======================================
  Files          74       74           
  Lines        5537     5546    +9     
  Branches     5537     5546    +9     
=======================================
+ Hits         4619     4627    +8     
- Misses        803      806    +3     
+ Partials      115      113    -2     

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

🚀 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.

@neon-bindings neon-bindings deleted a comment from github-actions bot Mar 28, 2025
Copy link

🐰 Bencher Report

Branchkv/no-args
Testbedubuntu-latest

🚨 10 Alerts

BenchmarkMeasure
Units
ViewBenchmark Result
(Result Δ%)
Lower Boundary
(Limit %)
Upper Boundary
(Limit %)
JsFunction::bindLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
198.67 ns
(-5.78%)Baseline: 210.86 ns
208.75 ns
(105.07%)

421.73 ns
(47.11%)
JsFunction::bindThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
4,914,643.21 ops/s
(+4.69%)Baseline: 4,694,405.32 ops/s
0.00 ops/s
(0.00%)
4,741,349.37 ops/s
(103.65%)

JsFunction::call_withLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
202.07 ns
(-3.39%)Baseline: 209.17 ns
207.07 ns
(102.48%)

418.33 ns
(48.30%)
JsFunction::call_withThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
4,861,292.81 ops/s
(+2.69%)Baseline: 4,733,974.83 ops/s
0.00 ops/s
(0.00%)
4,781,314.58 ops/s
(101.67%)

auto-exported-noopLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
28.04 ns
(-27.32%)Baseline: 38.58 ns
38.20 ns
(136.22%)

77.16 ns
(36.34%)
auto-exported-noopThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
34,981,675.55 ops/s
(+26.84%)Baseline: 27,578,719.75 ops/s
0.00 ops/s
(0.00%)
27,854,506.95 ops/s
(125.59%)

hello-worldLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
54.01 ns
(-2.51%)Baseline: 55.40 ns
54.85 ns
(101.55%)

110.81 ns
(48.75%)
hello-worldThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
18,471,275.47 ops/s
(+3.25%)Baseline: 17,889,450.26 ops/s
0.00 ops/s
(0.00%)
18,068,344.76 ops/s
(102.23%)

manually-exported-noopLatency
nanoseconds (ns)
📈 plot
🚷 threshold
🚨 alert (🔔)
28.15 ns
(-3.36%)Baseline: 29.13 ns
28.84 ns
(102.45%)

58.26 ns
(48.32%)
manually-exported-noopThroughput
operations / second (ops/s)
📈 plot
🚷 threshold
🚨 alert (🔔)
34,707,426.02 ops/s
(+2.21%)Baseline: 33,958,166.60 ops/s
0.00 ops/s
(0.00%)
34,297,748.27 ops/s
(101.19%)

Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Lower Boundary
nanoseconds (ns)
(Limit %)
Upper Boundary
nanoseconds (ns)
(Limit %)
ThroughputBenchmark Result
operations / second (ops/s)
(Result Δ%)
Lower Boundary
operations / second (ops/s)
(Limit %)
Upper Boundary
operations / second (ops/s)
(Limit %)
JsFunction::bind📈 view plot
🚷 view threshold
🚨 view alert (🔔)
198.67 ns
(-5.78%)Baseline: 210.86 ns
208.75 ns
(105.07%)

421.73 ns
(47.11%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
4,914,643.21 ops/s
(+4.69%)Baseline: 4,694,405.32 ops/s
0.00 ops/s
(0.00%)
4,741,349.37 ops/s
(103.65%)

JsFunction::call📈 view plot
🚷 view threshold
226.99 ns
(+0.63%)Baseline: 225.57 ns
223.31 ns
(98.38%)
451.14 ns
(50.32%)
📈 view plot
🚷 view threshold
4,402,536.07 ops/s
(-0.65%)Baseline: 4,431,529.03 ops/s
0.00 ops/s
(0.00%)
4,475,844.32 ops/s
(98.36%)
JsFunction::call_with📈 view plot
🚷 view threshold
🚨 view alert (🔔)
202.07 ns
(-3.39%)Baseline: 209.17 ns
207.07 ns
(102.48%)

418.33 ns
(48.30%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
4,861,292.81 ops/s
(+2.69%)Baseline: 4,733,974.83 ops/s
0.00 ops/s
(0.00%)
4,781,314.58 ops/s
(101.67%)

auto-exported-noop📈 view plot
🚷 view threshold
🚨 view alert (🔔)
28.04 ns
(-27.32%)Baseline: 38.58 ns
38.20 ns
(136.22%)

77.16 ns
(36.34%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
34,981,675.55 ops/s
(+26.84%)Baseline: 27,578,719.75 ops/s
0.00 ops/s
(0.00%)
27,854,506.95 ops/s
(125.59%)

hello-world📈 view plot
🚷 view threshold
🚨 view alert (🔔)
54.01 ns
(-2.51%)Baseline: 55.40 ns
54.85 ns
(101.55%)

110.81 ns
(48.75%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
18,471,275.47 ops/s
(+3.25%)Baseline: 17,889,450.26 ops/s
0.00 ops/s
(0.00%)
18,068,344.76 ops/s
(102.23%)

manually-exported-noop📈 view plot
🚷 view threshold
🚨 view alert (🔔)
28.15 ns
(-3.36%)Baseline: 29.13 ns
28.84 ns
(102.45%)

58.26 ns
(48.32%)
📈 view plot
🚷 view threshold
🚨 view alert (🔔)
34,707,426.02 ops/s
(+2.21%)Baseline: 33,958,166.60 ops/s
0.00 ops/s
(0.00%)
34,297,748.27 ops/s
(101.19%)

🐰 View full continuous benchmarking report in Bencher

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

1 participant