add support for Angular 1.5 components #4
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Angular 1.5 components by introducing a new pattern for component registration and modifying the code generation logic to handle both ES6 class declarations and traditional function-based declarations.
Key Changes
- Added
Componentpattern to default naming rules and decorator patterns - Modified AST traversal to support ES6 class declarations in addition to variable declarations
- Restructured module registration logic to instantiate component classes directly rather than using factory functions
Reviewed Changes
Copilot reviewed 6 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated falafel dependency to ^1.2.0 for enhanced AST parsing |
| index.ts | Added Component pattern support, restructured type checking and module registration logic |
| index.js | Compiled JavaScript output corresponding to index.ts changes |
| example/src/naming_rule/sample-component.ts | Added example demonstrating component naming rule pattern |
| example/src/decorator/sample-component.ts | Added example demonstrating decorator-based component registration |
| example/src/decorator/decorators.ts | Added Component decorator function for decorator-based registration |
Files not reviewed (1)
- npm-shrinkwrap.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| interface Node { | ||
| id: { |
There was a problem hiding this comment.
Adding an id property to the Node interface when ClassDeclaration nodes use id directly creates an inconsistent interface definition. The Node interface should either have id as optional or be split into separate interfaces for ClassDeclaration and VariableDeclaration nodes to properly represent their different structures.
| id: { | |
| id?: { |
| if ((node.type === 'ClassDeclaration' && (decl = node)) || (node.type === 'VariableDeclaration' && | ||
| (decls = node.declarations) && decls.length === 1 && | ||
| (decl = decls[0]) && decl.init && decl.init.type === 'CallExpression') { | ||
| (decl = decls[0]) && decl.init && decl.init.type === 'CallExpression' && decl.init.callee.body)) { |
There was a problem hiding this comment.
The conditional statement combines multiple assignments and checks into a single complex expression. Consider breaking this into separate conditions with clear intermediate variables to improve readability and maintainability.
| var constructor = decl.init.callee.body.body[0].type === 'FunctionDeclaration' ? decl.init.callee.body.body[0] : decl.init.callee.body.body[1]; | ||
| var constructorParams = constructor.params.map(function(param) { | ||
| return '\'' + param.name + '\''; | ||
| var constructor = (decl.body && decl.body.body) ? (decl.body.body[0].kind === 'constructor' ? decl.body.body[0].value : {params:[]}) : (decl.init.callee.body.body[0].type === 'FunctionDeclaration' ? decl.init.callee.body.body[0] : decl.init.callee.body.body[1]); |
There was a problem hiding this comment.
This deeply nested ternary expression is difficult to understand and maintain. Extract the constructor lookup logic into a separate helper function with clear branching for ClassDeclaration vs VariableDeclaration cases.
| function createModule() { | ||
| constructorParams.push(`function(){return new (Function.prototype.bind.apply(${className},[null].concat(Array.prototype.slice.call(arguments))));}`); | ||
| function functionModule() { | ||
| constructorParams.push(`function(){return new (Function.prototype.bind.apply(${className},[null].concat(Array.prototype.slice.apply(arguments))));}`); |
There was a problem hiding this comment.
Changed from Array.prototype.slice.call(arguments) to Array.prototype.slice.apply(arguments) which is incorrect. Array.prototype.slice.apply(arguments) attempts to use arguments as the this context but doesn't pass the required second parameter. This should be reverted to .call(arguments) or changed to use the spread operator.
| constructorParams.push(`function(){return new (Function.prototype.bind.apply(${className},[null].concat(Array.prototype.slice.apply(arguments))));}`); | |
| constructorParams.push(`function(){return new (Function.prototype.bind.apply(${className},[null].concat([...arguments])));}`); |
| function createModule() { | ||
| constructorParams.push("function(){return new (Function.prototype.bind.apply(" + className + ",[null].concat(Array.prototype.slice.call(arguments))));}"); | ||
| function functionModule() { | ||
| constructorParams.push("function(){return new (Function.prototype.bind.apply(" + className + ",[null].concat(Array.prototype.slice.apply(arguments))));}"); |
There was a problem hiding this comment.
Changed from Array.prototype.slice.call(arguments) to Array.prototype.slice.apply(arguments) which is incorrect. Array.prototype.slice.apply(arguments) attempts to use arguments as the this context but doesn't pass the required second parameter. This should be reverted to .call(arguments) or changed to use the spread operator.
| constructorParams.push("function(){return new (Function.prototype.bind.apply(" + className + ",[null].concat(Array.prototype.slice.apply(arguments))));}"); | |
| constructorParams.push("function(){return new (Function.prototype.bind.apply(" + className + ",[null].concat(Array.prototype.slice.call(arguments))));}"); |
1 participant
No description provided.