Skip to content

Conversation

@weareoutman
Copy link
Member

No description provided.

@weareoutman weareoutman requested a review from Copilot November 11, 2025 08:10
@weareoutman weareoutman merged commit 7690c42 into master Nov 11, 2025
5 checks passed
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 adds support for event callback declarations in component code, allowing developers to define event handlers as separate functions instead of inline arrow functions. The changes introduce a new binding kind eventCallback and rename the existing eventHandler to eventHandlerParam for better clarity.

Key Changes

  • Introduces eventCallback binding kind to support named function declarations as event handlers
  • Renames eventHandler to eventHandlerParam to distinguish parameter-based handlers from callback-based handlers
  • Adds validation for multiple parameters in render callbacks for custom elements

Reviewed Changes

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

Show a summary per file
File Description
packages/parser/src/modules/parseJsValue.ts Adds validation to reject multiple parameters in render callbacks for custom elements
packages/parser/src/modules/parseJSXElement.ts Adds switch case to handle eventCallback binding kind and route to appropriate parser
packages/parser/src/modules/parseEvent.ts Renames eventHandler to eventHandlerParam for consistency
packages/parser/src/modules/parseComponent.ts Implements detection and registration of event callback functions with block statements
packages/parser/src/modules/interfaces.ts Defines new eventCallback binding kind and adds callback field to BindingInfo
packages/parser/src/snapshots/parseView.spec.ts.snap Updates snapshot with renamed binding kind
packages/parser/src/snapshots/parseTemplate.spec.ts.snap Updates snapshot with renamed binding kind
apps/test/src/Pages/Layout.tsx Demonstrates the new feature by refactoring inline event handler to named function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +322 to +329
// It's a event callback function
const funcBinding: BindingInfo = {
id: declId.node,
kind: "eventCallback",
callback: init,
};
bindingMap.set(declId.node, funcBinding);
continue;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

[nitpick] This logic treats all function expressions with block statements as event callbacks. However, this may be too broad - any constant function with a block body would be classified as an event callback, even if it's not intended for event handling. Consider adding additional validation to ensure the function is actually used as an event handler (e.g., checking if it's referenced in an onClick or similar event attribute), or document this behavior clearly if it's intentional.

Suggested change
// It's a event callback function
const funcBinding: BindingInfo = {
id: declId.node,
kind: "eventCallback",
callback: init,
};
bindingMap.set(declId.node, funcBinding);
continue;
// Only treat as event callback if variable name matches /^on[A-Z]/
const varName = declId.node.name;
if (/^on[A-Z]/.test(varName)) {
const funcBinding: BindingInfo = {
id: declId.node,
kind: "eventCallback",
callback: init,
};
bindingMap.set(declId.node, funcBinding);
continue;
}

Copilot uses AI. Check for mistakes.
if (isGeneralFunctionExpression(init)) {
const callbackBody = init.get("body");
if (callbackBody.isBlockStatement()) {
// It's a event callback function
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The comment has a grammatical error: "It's a event callback" should be "It's an event callback" (using "an" instead of "a" before a vowel sound).

Suggested change
// It's a event callback function
// It's an event callback function

Copilot uses AI. Check for mistakes.
];
break;
case "eventCallback":
eventPath = binding.callback!;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

Potential type mismatch: The parseEvent function (line 37) only accepts ArrowFunctionExpression, but binding.callback could be either a FunctionExpression or ArrowFunctionExpression according to the type definition in interfaces.ts. When binding.callback is a FunctionExpression, this will cause an error at runtime. Consider either:

  1. Restricting event callbacks to only arrow functions in parseComponent.ts (line 319)
  2. Updating parseEvent to handle both function types
Suggested change
eventPath = binding.callback!;
if (
t.isArrowFunctionExpression(binding.callback) ||
t.isFunctionExpression(binding.callback)
) {
eventPath = binding.callback;
} else {
state.errors.push({
message: `Event callback must be an arrow function or function expression`,
node: binding.callback,
severity: "error",
});
continue;
}

Copilot uses AI. Check for mistakes.
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