feat(napi/parser)!: represent empty optional fields on JS side as null#16411
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
This PR enhances NAPI-RS data transfer by representing empty optional fields as null instead of omitting them entirely. This aligns the representation with how AST fields are handled and improves JS engine optimization through consistent object shapes.
Key Changes:
- Added
use_nullable = trueto NAPI object macros for module record and error structs - Updated TypeScript type definitions to reflect
| nullinstead of optional fields (?) - Removed the
removeNullPropertieshelper function that was previously needed to normalize outputs - Updated all test snapshots to expect
nullvalues for empty optional fields
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
napi/parser/src/types.rs |
Added use_nullable = true to ImportName, StaticExportEntry, ExportImportName, ExportExportName, and ExportLocalName structs |
crates/oxc_napi/src/error.rs |
Added use_nullable = true to OxcError and ErrorLabel structs |
napi/parser/src-js/index.d.ts |
Updated TypeScript type definitions to use ` |
napi/parser/test/parse.test.ts |
Updated test expectations to include null values for empty optional fields |
napi/parser/test/parse-raw-worker.ts |
Removed removeNullProperties helper function and related field reordering logic |
napi/parser/test/__snapshots__/esm.test.ts.snap |
Updated all test snapshots to include null values for empty optional fields in module records |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
248b924 to
022bf48
Compare
022bf48 to
9c8d65e
Compare
Merge activity
|
…ll` (#16411) Follow-on after #16383 and #16403. While working on those I discovered an option NAPI-RS has to represent `Option::None` as `null`, instead of omitting the field entirely. Use that option on the structs used for transferring errors and module record over to JS. I think this is likely more performant because: 1. NAPI-RS can create all properties of the objects using only the faster `node_api_create_object_with_properties` API. 2. It produces consistent object shapes, so JS engine can better optimize code using these objects. It also brings the shape of the data perfectly into line between standard transfer and raw transfer, and is consistent with how empty fields in the AST are represented as `null`. I would have used this option before if I'd known it existed. ### Breaking change I've marked this as a breaking change because code consuming these objects would now need to check for empty fields with `value === null` instead of `value === undefined`. However in practice, most people probably use `!value` or `value ?? ...`, so it's unlikely to affect many users. This only affects module record and errors anyway, not the AST itself, as we transfer that as JSON, not via NAPI.
9c8d65e to
083fea9
Compare
…ll` (oxc-project#16411) Follow-on after oxc-project#16383 and oxc-project#16403. While working on those I discovered an option NAPI-RS has to represent `Option::None` as `null`, instead of omitting the field entirely. Use that option on the structs used for transferring errors and module record over to JS. I think this is likely more performant because: 1. NAPI-RS can create all properties of the objects using only the faster `node_api_create_object_with_properties` API. 2. It produces consistent object shapes, so JS engine can better optimize code using these objects. It also brings the shape of the data perfectly into line between standard transfer and raw transfer, and is consistent with how empty fields in the AST are represented as `null`. I would have used this option before if I'd known it existed. ### Breaking change I've marked this as a breaking change because code consuming these objects would now need to check for empty fields with `value === null` instead of `value === undefined`. However in practice, most people probably use `!value` or `value ?? ...`, so it's unlikely to affect many users. This only affects module record and errors anyway, not the AST itself, as we transfer that as JSON, not via NAPI.
…xc-project#16412) Revert the change made in oxc-project#16403. It's no longer necessary after oxc-project#16411, because NAPI-RS no longer re-orders the fields.
### 💥 BREAKING CHANGES - 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on JS side as `null` (#16411) (overlookmotel) ### 🚀 Features - 7a2afee parser: Add TS1174 error for classes extending multiple base classes (#15993) (sapphi-red) - da87812 semantic: Add TS2309 error for export assignment with other exports (#15992) (sapphi-red) - d6d2bcd minifier: Remove unused function calls that are marked by `manual_pure_functions` (#16534) (sapphi-red) - c90f053 minifier: Support `.` separated values for `compress.treeshake.manualPureFunctions` (#16529) (sapphi-red) - a607cc4 codegen: Preserve comments between CatchClause's param and body (#16167) (copilot-swe-agent) - 8c10694 semantic: Expose get_comment_at method (#16439) (camc314) - 3981e7a ast: Add get_comment_at to lookup a comment by span (#16438) (camc314) ### 🐛 Bug Fixes - 699406a napi/parser: Move `ExportEntry::module_request` field to first (#16412) (overlookmotel) - 12bd794 napi/parser: Move `ExportEntry::module_request` field to last (#16403) (overlookmotel) ### ⚡ Performance - 790beeb napi/parser: Do not remove extraneous options on JS side (#16447) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
### 💥 BREAKING CHANGES - 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on JS side as `null` (#16411) (overlookmotel) ### 🚀 Features - 7a2afee parser: Add TS1174 error for classes extending multiple base classes (#15993) (sapphi-red) - da87812 semantic: Add TS2309 error for export assignment with other exports (#15992) (sapphi-red) - d6d2bcd minifier: Remove unused function calls that are marked by `manual_pure_functions` (#16534) (sapphi-red) - c90f053 minifier: Support `.` separated values for `compress.treeshake.manualPureFunctions` (#16529) (sapphi-red) - a607cc4 codegen: Preserve comments between CatchClause's param and body (#16167) (copilot-swe-agent) - 8c10694 semantic: Expose get_comment_at method (#16439) (camc314) - 3981e7a ast: Add get_comment_at to lookup a comment by span (#16438) (camc314) ### 🐛 Bug Fixes - 699406a napi/parser: Move `ExportEntry::module_request` field to first (#16412) (overlookmotel) - 12bd794 napi/parser: Move `ExportEntry::module_request` field to last (#16403) (overlookmotel) ### ⚡ Performance - 790beeb napi/parser: Do not remove extraneous options on JS side (#16447) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
…ll` (oxc-project#16411) Follow-on after oxc-project#16383 and oxc-project#16403. While working on those I discovered an option NAPI-RS has to represent `Option::None` as `null`, instead of omitting the field entirely. Use that option on the structs used for transferring errors and module record over to JS. I think this is likely more performant because: 1. NAPI-RS can create all properties of the objects using only the faster `node_api_create_object_with_properties` API. 2. It produces consistent object shapes, so JS engine can better optimize code using these objects. It also brings the shape of the data perfectly into line between standard transfer and raw transfer, and is consistent with how empty fields in the AST are represented as `null`. I would have used this option before if I'd known it existed. ### Breaking change I've marked this as a breaking change because code consuming these objects would now need to check for empty fields with `value === null` instead of `value === undefined`. However in practice, most people probably use `!value` or `value ?? ...`, so it's unlikely to affect many users. This only affects module record and errors anyway, not the AST itself, as we transfer that as JSON, not via NAPI.
…xc-project#16412) Revert the change made in oxc-project#16403. It's no longer necessary after oxc-project#16411, because NAPI-RS no longer re-orders the fields.
### 💥 BREAKING CHANGES - 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on JS side as `null` (oxc-project#16411) (overlookmotel) ### 🚀 Features - 7a2afee parser: Add TS1174 error for classes extending multiple base classes (oxc-project#15993) (sapphi-red) - da87812 semantic: Add TS2309 error for export assignment with other exports (oxc-project#15992) (sapphi-red) - d6d2bcd minifier: Remove unused function calls that are marked by `manual_pure_functions` (oxc-project#16534) (sapphi-red) - c90f053 minifier: Support `.` separated values for `compress.treeshake.manualPureFunctions` (oxc-project#16529) (sapphi-red) - a607cc4 codegen: Preserve comments between CatchClause's param and body (oxc-project#16167) (copilot-swe-agent) - 8c10694 semantic: Expose get_comment_at method (oxc-project#16439) (camc314) - 3981e7a ast: Add get_comment_at to lookup a comment by span (oxc-project#16438) (camc314) ### 🐛 Bug Fixes - 699406a napi/parser: Move `ExportEntry::module_request` field to first (oxc-project#16412) (overlookmotel) - 12bd794 napi/parser: Move `ExportEntry::module_request` field to last (oxc-project#16403) (overlookmotel) ### ⚡ Performance - 790beeb napi/parser: Do not remove extraneous options on JS side (oxc-project#16447) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

Follow-on after #16383 and #16403.
While working on those I discovered an option NAPI-RS has to represent
Option::Noneasnull, instead of omitting the field entirely.Use that option on the structs used for transferring errors and module record over to JS.
I think this is likely more performant because:
node_api_create_object_with_propertiesAPI.It also brings the shape of the data perfectly into line between standard transfer and raw transfer, and is consistent with how empty fields in the AST are represented as
null.I would have used this option before if I'd known it existed.
Breaking change
I've marked this as a breaking change because code consuming these objects would now need to check for empty fields with
value === nullinstead ofvalue === undefined.However in practice, most people probably use
!valueorvalue ?? ..., so it's unlikely to affect many users. This only affects module record and errors anyway, not the AST itself, as we transfer that as JSON, not via NAPI.