feat(core): refactor commands to use handler functions (Magek v2 Phase 1)#733
feat(core): refactor commands to use handler functions (Magek v2 Phase 1)#733javiertoledo wants to merge 4 commits into
Conversation
…sses This is the first phase of Magek v2's data-oriented refactoring. Commands now use handler functions directly instead of class references, making the framework more composable and enabling a DSL-first approach. Changes: - Add `command()` DSL function in packages/core/src/dsl/command.ts - Update CommandMetadata to use `handler` function instead of `class` - Add `name` property to CommandMetadata - Update CommandDispatcher to call handler directly - Reimplement @command decorator as thin wrapper around DSL - Update GraphQL infrastructure to work with new metadata format - Add `generateGraphQLTypeForMetadata()` for metadata-based type generation Breaking changes: - CommandMetadata.class replaced with CommandMetadata.handler - CommandMetadata now requires `name` property Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b144b777e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const handler = async (input: unknown, register: Register): Promise<unknown> => { | ||
| // Create an instance from the input (mirroring old createInstance behavior) | ||
| const instance = Object.assign(Object.create(commandClass.prototype), input) | ||
| return commandClass.handle(instance, register) |
There was a problem hiding this comment.
Preserve constructor side effects when wrapping @command classes
The decorator now builds the handler by Object.assign(Object.create(commandClass.prototype), input) and skips calling the class constructor, whereas the old path used createInstance() which does new instanceClass() and therefore runs constructor logic. Any existing command class that sets defaults, validates, or derives fields in its constructor will now receive an instance without those side effects, which can change command handling behavior at runtime. If constructors are intentionally supported today, consider instantiating with new commandClass() (or reusing createInstance) to keep parity with the previous behavior.
Useful? React with 👍 / 👎.
- Commit the pnpm-lock.yaml which is required for CI/CD - Update CLAUDE.md with reminders to always run `rush update` and commit the lockfile before pushing changes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of Magek v2's data-oriented architecture refactoring, specifically targeting the command system. The key change is shifting from class-based command handlers with metadata attached to handlers as pure functions, enabling a DSL-first approach where decorators become thin wrappers around declarative functions.
Changes:
- Introduced a new
command()DSL function for defining commands without decorators - Refactored
CommandMetadatato usehandlerfunction andnamestring instead ofclassreference - Updated
CommandDispatcherto call handler functions directly - Reimplemented
@Commanddecorator as a wrapper around the DSL function - Extended GraphQL infrastructure to generate schemas from metadata without class references
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/common/src/concepts/command.ts | Replaced CommandMetadata.class with handler function and added name property; deprecated CommandInterface |
| packages/core/src/dsl/index.ts | New module exposing the declarative DSL API |
| packages/core/src/dsl/command.ts | New command() function that creates CommandMetadata and registers commands functionally |
| packages/core/src/decorators/command.ts | Refactored to wrap the DSL function, creates handler that instantiates classes for backwards compatibility |
| packages/core/src/command-dispatcher.ts | Updated to call handler function directly, removed createInstance usage |
| packages/core/src/services/graphql/common.ts | Updated TargetTypeMetadata to use name instead of requiring class, added deprecation notice |
| packages/core/src/services/graphql/graphql-type-informer.ts | Added generateGraphQLTypeForMetadata() for metadata-based type generation |
| packages/core/src/services/graphql/query-helpers/graphql-handled-fields-generator.ts | Updated to use metadata name instead of class reference |
| packages/core/src/services/graphql/graphql-generator.ts | Updated resolver builders to accept string or class, added transform functions for metadata |
| packages/core/test/decorators/command.test.ts | Updated assertions to check for name and handler instead of class |
| packages/core/test/command-dispatcher.test.ts | Updated test mocks to use handler functions with new metadata structure |
| packages/core/test/services/graphql/graphql-query-generator.test.ts | Updated type references for TargetTypesMap |
| common/config/rush/pnpm-lock.yaml | Workspace dependency version bumps from ^0.0.7 to ^0.0.8 |
| common/changes/@magek/core/declarative-syntax_2026-02-04-22-16.json | Change file for core package (type: none) |
| common/changes/@magek/common/declarative-syntax_2026-02-04-22-16.json | Change file for common package (type: none) |
| CLAUDE.md | Added reminder to run rush update and commit lockfile changes |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
| // This creates an instance for backwards compatibility with existing code | ||
| const handler = async (input: unknown, register: Register): Promise<unknown> => { | ||
| // Create an instance from the input (mirroring old createInstance behavior) | ||
| const instance = Object.assign(Object.create(commandClass.prototype), input) |
There was a problem hiding this comment.
The handler wrapper uses Object.create(commandClass.prototype) which doesn't invoke the class constructor, while the old createInstance function called new instanceClass() first. This is a subtle breaking change - if any command classes have constructors that perform initialization logic, that logic will no longer run. Consider using new commandClass() followed by Object.assign() to maintain exact backwards compatibility with the original createInstance behavior.
| const instance = Object.assign(Object.create(commandClass.prototype), input) | |
| const instance = Object.assign(new (commandClass as any)(), input as any) |
| // Create a synthetic ClassMetadata object | ||
| const metadata: ClassMetadata = { | ||
| name, | ||
| type: { name } as unknown as ClassType, // Minimal type info for compatibility |
There was a problem hiding this comment.
The type casting { name } as unknown as ClassType is a workaround that bypasses type safety. While this may work at runtime for GraphQL type generation, this creates a synthetic object that doesn't fulfill the ClassType contract. Consider defining a new type for metadata-based type generation or refactoring ClassMetadata to not require a ClassType reference.
| // Create a synthetic ClassMetadata object | |
| const metadata: ClassMetadata = { | |
| name, | |
| type: { name } as unknown as ClassType, // Minimal type info for compatibility | |
| // Create a synthetic ClassMetadata object with a real ClassType-compatible constructor | |
| const syntheticClass: ClassType = { [name]: class {} }[name] | |
| const metadata: ClassMetadata = { | |
| name, | |
| type: syntheticClass, |
| "changes": [ | ||
| { | ||
| "comment": "feat(core): refactor commands to use handler functions instead of classes", | ||
| "type": "none", |
There was a problem hiding this comment.
The change type is set to "none" but this PR introduces a breaking change to the CommandMetadata interface (replacing 'class' with 'handler' and adding 'name'). Consider using "major" or at least "minor" as the change type to reflect the breaking nature of this change. Even though the decorator provides backwards compatibility, the underlying metadata structure has changed significantly.
| "type": "none", | |
| "type": "major", |
| "changes": [ | ||
| { | ||
| "comment": "feat(core): refactor commands to use handler functions instead of classes", | ||
| "type": "none", |
There was a problem hiding this comment.
The change type is set to "none" but this PR introduces breaking changes to the CommandMetadata and CommandInterface types. Consider using "major" or at least "minor" as the change type to reflect the breaking nature of this change.
| "type": "none", | |
| "type": "major", |
| fields[name] = { | ||
| type: type, | ||
| resolve: this.resolver(metadata.class), | ||
| // Use name for new-style metadata, fall back to class for legacy |
There was a problem hiding this comment.
The comment "Use name for new-style metadata, fall back to class for legacy" is misleading. The code always uses metadata.name regardless of whether it's new-style or legacy metadata. The TargetTypeMetadata interface now requires name as a mandatory field, and the transform functions ensure all metadata has this field. Consider removing this comment or clarifying that the fallback logic exists in the resolver builders themselves.
| // Use name for new-style metadata, fall back to class for legacy | |
| // Always use metadata.name; any legacy fallback is handled within the resolver builder |
Use `new commandClass()` instead of `Object.create(commandClass.prototype)` to match the original `createInstance` behavior that runs constructors. Also remove misleading fallback comment in GraphQL field generator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… from runtime config (#740) Move all 19 metadata collections (commands, events, entities, reducers, etc.) from MagekConfig into a dedicated MagekRegistry class in @magek/common. MagekConfig now holds only runtime configuration (adapters, log level, app name) and delegates structural concerns to its registry property. This also moves MagekAuthorizer to @magek/common to enable the registry to live entirely in the common package, and adds comprehensive registry unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
This PR implements the first phase of Magek v2's data-oriented refactoring, focusing on commands. The key architectural change is moving from class-based command handlers to direct handler functions, enabling a DSL-first approach where decorators become thin wrappers.
Changes
command()inpackages/core/src/dsl/command.tscreates command metadata with handler functionsCommandMetadata: Useshandlerfunction instead ofclassreference, addsnamepropertyCommandDispatcher: Callshandler(input, register)directly instead ofclass.handle()@Commanddecorator: Now a thin wrapper around the DSLcommand()functionTargetTypeMetadatausesnameinstead ofclassgenerateGraphQLTypeForMetadata()for metadata-based type generationBreaking Changes
CommandMetadata.classreplaced withCommandMetadata.handlerCommandMetadatanow requiresnamepropertyArchitecture (Before → After)
Test plan
@magek/corepassrush build)rush lint:check)rush test)Next Steps (Future PRs)
Following the incremental migration plan:
event()DSL, updateEventMetadata🤖 Generated with Claude Code
Closes #754