Skip to content

fix(napi-derive): using js_name generating wrong type defs#2700

Merged
Brooooooklyn merged 1 commit into
mainfrom
feat/js-name-struct-test
Jun 8, 2025
Merged

fix(napi-derive): using js_name generating wrong type defs#2700
Brooooooklyn merged 1 commit into
mainfrom
feat/js-name-struct-test

Conversation

@Brooooooklyn

@Brooooooklyn Brooooooklyn commented Jun 8, 2025

Copy link
Copy Markdown
Member

This commit introduces two main changes:

  1. Refactors the variable names used during the creation of NapiStruct in crates/macro/src/parser/mod.rs. The goal is to make the source of NapiStruct.name (the Rust identifier) and NapiStruct.js_name (the JavaScript name, potentially from a #[napi(js_name = "...")] attribute) more explicit. This change is primarily for code readability and maintainability. The core logic remains the same.

  2. Adds a new test case to examples/napi to specifically verify the behavior of #[napi(js_name = "...")] on structs. This includes:

    • A new Rust struct OriginalRustNameForJsNamedStruct in examples/napi/src/class.rs decorated with #[napi(js_name = "MyJsNamedClass")].
    • Corresponding tests in examples/napi/__tests__/values.spec.ts that check for correct class instantiation, method calls, and type alias generation (OriginalRustNameForJsNamedStruct as an alias for MyJsNamedClass).

This commit introduces two main changes:

1.  Refactors the variable names used during the creation of
    `NapiStruct` in `crates/macro/src/parser/mod.rs`. The goal is to
    make the source of `NapiStruct.name` (the Rust identifier) and
    `NapiStruct.js_name` (the JavaScript name, potentially from a
    `#[napi(js_name = "...")]` attribute) more explicit. This change
    is primarily for code readability and maintainability. The core
    logic remains the same.

2.  Adds a new test case to `examples/napi` to specifically verify
    the behavior of `#[napi(js_name = "...")]` on structs. This
    includes:
    - A new Rust struct `OriginalRustNameForJsNamedStruct` in
      `examples/napi/src/class.rs` decorated with
      `#[napi(js_name = "MyJsNamedClass")]`.
    - Corresponding tests in `examples/napi/__tests__/values.spec.ts`
      that check for correct class instantiation, method calls, and
      type alias generation (`OriginalRustNameForJsNamedStruct` as an
      alias for `MyJsNamedClass`).

Note: I encountered some issues running the full build and tests for the example due to pre-commit hooks in the testing environment that I couldn't bypass. However, I believe the changes are correct based on static analysis and focused checks.
@Brooooooklyn Brooooooklyn force-pushed the feat/js-name-struct-test branch from 30484a8 to 824d7c5 Compare June 8, 2025 09:01

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Brooooooklyn Brooooooklyn requested a review from Copilot June 8, 2025 09:02
@Brooooooklyn Brooooooklyn marked this pull request as ready for review June 8, 2025 09:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the assignment of JavaScript names to Rust structs in the NAPI binding generator and adds new tests to verify the behavior of the js_name attribute.

  • Renames variables for better clarity in NapiStruct construction in crates/macro/src/parser/mod.rs.
  • Introduces two new structs in examples/napi/src/class.rs with the js_name attribute and updates related type definitions and module exports.
  • Adds comprehensive tests in examples/napi/tests/values.spec.ts and updates snapshot and module declaration files accordingly.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
examples/napi/src/class.rs Added new struct definitions with js_name and corresponding methods.
examples/napi/index.d.cts Updated type declarations and type aliases for new structs.
examples/napi/index.cjs, example.wasi.* Updated module exports to include new structs.
examples/napi/tests/values.spec.ts Added tests to verify js_name behavior and type compatibility.
crates/macro/src/parser/mod.rs Refactored variable names related to js_name assignment in NapiStruct.
Comments suppressed due to low confidence (1)

crates/macro/src/parser/mod.rs:1374

  • [nitpick] Consider using a consistent casing function for default js name generation (e.g., using Case::Pascal instead of Case::UpperCamel) across all implementations for clarity, even if they are equivalent in this context.
let mut struct_js_name = struct_name.to_string().to_case(Case::UpperCamel);

@Brooooooklyn

Copy link
Copy Markdown
Member Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ BugBot reviewed your changes and found no bugs!


BugBot free trial expires on June 14, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

@Brooooooklyn Brooooooklyn changed the title Refactor NapiStruct js_name assignment and add test fix(napi-derive): using js_name generating wrong type defs Jun 8, 2025
@Brooooooklyn Brooooooklyn merged commit 814bc79 into main Jun 8, 2025
64 checks passed
@Brooooooklyn Brooooooklyn deleted the feat/js-name-struct-test branch June 8, 2025 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants