Skip to content

Conversation

@kraenhansen
Copy link
Collaborator

As a preparation for another PR adding more generators.

Copy link
Contributor

Copilot AI left a comment

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 code generation for the weak-node-api package by extracting generator functions into separate files, preparing the codebase for additional generators to be added in a future PR.

Key Changes:

  • Extracted generator functions from generate-weak-node-api.ts into dedicated generator modules
  • Introduced a shared utility module for common generation logic
  • Enhanced error handling for the clang-format subprocess call

Reviewed Changes

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

File Description
packages/weak-node-api/scripts/generators/weak-node-api.ts New file containing the extracted generateHeader and generateSource generator functions
packages/weak-node-api/scripts/generators/shared.ts New file with shared generateFunction utility for C/C++ function code generation
packages/weak-node-api/scripts/generate-weak-node-api.ts Updated to import and use the extracted generators; improved error handling for clang-format

return `
#pragma once
#include <node_api.h> // Node-API
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] Corrected comment: 'Node-API' should be 'Node-API headers' for consistency with other include comments.

Suggested change
#include <node_api.h> // Node-API
#include <node_api.h> // Node-API headers

Copilot uses AI. Check for mistakes.
* Generates source code for a version script for the given Node API version.
*/
export function generateHeader(functions: FunctionDecl[]) {
return `
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The header lacks a #pragma once include guard comment explaining its purpose. The generated header should include a brief description of what this file provides (e.g., 'Weak Node-API host injection interface').

Suggested change
return `
return `
/**
* @file weak_node_api.hpp
* @brief Weak Node-API host injection interface.
*
* This header provides the struct and injection function for
* dynamically supplying Node-API function pointers to the host.
*
* #pragma once is used as an include guard to prevent multiple inclusion.
*/

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll be adding this in a followup PR.

const { status, stderr } = cp.spawnSync("clang-format", ["-i", outputPath], {
encoding: "utf8",
});
assert.equal(status, 0, `Failed to format ${fileName}: ${stderr}`);
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The error message will display 'undefined' when stderr is empty. The spawnSync options specify encoding: 'utf8', but when status is non-zero and stderr is empty, this will show an unhelpful message. Consider using stderr || 'no error output' or checking if stderr exists before interpolating it.

Suggested change
assert.equal(status, 0, `Failed to format ${fileName}: ${stderr}`);
assert.equal(status, 0, `Failed to format ${fileName}: ${stderr || "no error output"}`);

Copilot uses AI. Check for mistakes.
@kraenhansen kraenhansen force-pushed the kh/weak-node-api-refactored-generator-2 branch from fe4d1f2 to 845f6bb Compare November 10, 2025 21:25
@kraenhansen kraenhansen merged commit a0163f5 into main Nov 10, 2025
11 checks passed
@kraenhansen kraenhansen deleted the kh/weak-node-api-refactored-generator-2 branch November 10, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants