-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Phase 1 - Establish plugin architecture foundation with @objectql/types v4.0.0 #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Phase 1 - Establish plugin architecture foundation with @objectql/types v4.0.0 #150
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
6946f22
into
copilot/evaluate-code-migration
There was a problem hiding this 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 pull request implements Phase 1 of the ObjectQL v4.0 migration, establishing the foundation for transitioning from a monolithic framework to a plugin-based architecture. The PR creates a new @objectql/types v4.0.0 package with plugin-first type definitions while maintaining backward compatibility with existing v3.x packages.
Changes:
- Created new
@objectql/typesv4.0.0 package with plugin interfaces (QueryProcessorPlugin, RepositoryPlugin, BasePlugin) and query types (UnifiedQuery, FilterExpression, QueryResult) - Added workspace configuration for
packages/core/*andpackages/plugins/*directories - Established plugin architecture foundation with runtime context and lifecycle hooks
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Added reference to new packages/core/types package for composite builds |
| pnpm-workspace.yaml | Added packages/core/* and packages/plugins/* to workspace packages |
| packages/plugins/README.md | Documentation for plugin directory structure and development guidelines |
| packages/core/types/tsconfig.json | TypeScript configuration extending base config |
| packages/core/types/src/repository.ts | RepositoryPlugin interface with CRUD lifecycle hooks |
| packages/core/types/src/query.ts | Core query types (UnifiedQuery, FilterExpression, QueryResult, QueryOptions) |
| packages/core/types/src/query-processor.ts | QueryProcessorPlugin interface with validation, optimization, and transformation hooks |
| packages/core/types/src/plugin.ts | BasePlugin interface with metadata and lifecycle methods |
| packages/core/types/src/index.ts | Main entry point exporting all plugin and query types |
| packages/core/types/package.json | Package configuration with dependencies on @objectstack packages |
| packages/core/types/package-lock.json | NPM lock file (should not be present in PNPM monorepo) |
| packages/core/types/jest.config.js | Jest test configuration |
| packages/core/types/README.md | Package documentation and usage examples |
| packages/core/types/CHANGELOG.md | Version 4.0.0 changelog with migration guide |
| packages/core/types/.gitignore | Standard ignores for build artifacts |
| PHASE_1_COMPLETE.md | Documentation of Phase 1 completion and next steps |
Files not reviewed (1)
- packages/core/types/package-lock.json: Language not supported
| * @param context - Execution context | ||
| */ | ||
| extendRepository?( | ||
| repository: any, |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type here violates the strict typing principle stated in the coding guidelines and the package's own documentation. While the comment explains this is to avoid circular dependencies, a better approach would be to use proper TypeScript techniques like generic type parameters or importing specific types from @objectstack/runtime. Consider defining a minimal RuntimeInterface type that can be imported without creating circular dependencies.
| beforeUpdate?( | ||
| id: string | number, | ||
| data: any, | ||
| context: RuntimeContext | ||
| ): Promise<any>; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for data parameters violates strict typing principles. Consider using a generic type parameter to maintain type safety.
| beforeQuery?( | ||
| ast: any, | ||
| context: RuntimeContext | ||
| ): Promise<any>; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for AST parameters and return values violates strict typing principles. Consider defining a QueryAST type to maintain type safety throughout the query transformation pipeline.
| optimizeQuery?( | ||
| ast: any, | ||
| context: RuntimeContext | ||
| ): Promise<any>; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for AST parameters and return values violates strict typing principles. Consider defining a QueryAST type to maintain type safety.
| /** Query being executed (set by beforeQuery) */ | ||
| query?: any; | ||
|
|
||
| /** Flag to skip query execution (e.g., for cache hits) */ | ||
| skipExecution?: boolean; | ||
|
|
||
| /** Cached result (if skipExecution is true) */ | ||
| cachedResult?: any; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for the query and cachedResult properties violates strict typing principles. Consider using more specific types like UnifiedQuery (defined in this package) for the query property, and a generic type parameter for cachedResult.
| /** | ||
| * Re-export commonly used types from @objectstack/runtime | ||
| * This provides a convenient import path for ObjectQL users. | ||
| */ | ||
| export type { | ||
| // RuntimePlugin, | ||
| // Runtime, | ||
| // Add other runtime types as needed | ||
| } from '@objectstack/runtime'; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out re-exports suggest incomplete implementation. If these types from @objectstack/runtime are needed for the public API, they should be uncommented and properly exported. If they're not needed yet, consider removing the commented lines to keep the code clean.
| /** | |
| * Re-export commonly used types from @objectstack/runtime | |
| * This provides a convenient import path for ObjectQL users. | |
| */ | |
| export type { | |
| // RuntimePlugin, | |
| // Runtime, | |
| // Add other runtime types as needed | |
| } from '@objectstack/runtime'; |
| afterQuery?( | ||
| result: any, | ||
| context: RuntimeContext | ||
| ): Promise<any>; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for result parameters and return values violates strict typing principles. Consider using a generic type parameter or the QueryResult type defined in this package to maintain type safety.
| */ | ||
| export interface QueryOptions { | ||
| /** Transaction to execute within */ | ||
| transaction?: any; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type for the transaction property violates strict typing principles. Consider using unknown or importing a Transaction type from the appropriate @objectstack package to maintain type safety.
| transaction?: any; | |
| transaction?: unknown; |
| allowUnknownFields?: boolean; | ||
|
|
||
| /** Custom validator functions */ | ||
| customValidators?: Record<string, (value: any) => boolean>; |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The customValidators property uses any for the validator function's parameter type, violating strict typing principles. Consider using unknown instead, which would force consumers to properly type-guard the value before using it.
| customValidators?: Record<string, (value: any) => boolean>; | |
| customValidators?: Record<string, (value: unknown) => boolean>; |
| { | ||
| "name": "@objectql/types", | ||
| "version": "4.0.0", | ||
| "lockfileVersion": 3, | ||
| "requires": true, | ||
| "packages": { | ||
| "": { | ||
| "name": "@objectql/types", | ||
| "version": "4.0.0", | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@objectstack/runtime": "^0.2.0", | ||
| "@objectstack/spec": "^0.2.0" | ||
| } | ||
| }, | ||
| "node_modules/@objectstack/objectql": { | ||
| "version": "0.2.0", | ||
| "resolved": "https://registry.npmjs.org/@objectstack/objectql/-/objectql-0.2.0.tgz", | ||
| "integrity": "sha512-LCgCs+K7J4/rOwJZdXFZgm+zVD936ppRgRVwYn3Uc8xP/JZeFg/DDrhW+O1li3Enwbai2eKMCRB6XYinOhoCAg==", | ||
| "dependencies": { | ||
| "@objectstack/spec": "0.2.0" | ||
| } | ||
| }, | ||
| "node_modules/@objectstack/runtime": { | ||
| "version": "0.2.0", | ||
| "resolved": "https://registry.npmjs.org/@objectstack/runtime/-/runtime-0.2.0.tgz", | ||
| "integrity": "sha512-nKDm3HSbGDkpccGKXDXhOr3nvEOgz6cp1j/z74DoreVD/6gIH6PuWPldHJbOVdR/nhPHNV/ViK7tvGmLML2v1A==", | ||
| "dependencies": { | ||
| "@objectstack/objectql": "0.2.0", | ||
| "@objectstack/spec": "0.2.0", | ||
| "@objectstack/types": "0.2.0" | ||
| } | ||
| }, | ||
| "node_modules/@objectstack/spec": { | ||
| "version": "0.2.0", | ||
| "resolved": "https://registry.npmjs.org/@objectstack/spec/-/spec-0.2.0.tgz", | ||
| "integrity": "sha512-y4JALcsTgOeEE0xRJ7Co16beQTIO42c6KngFFcy7QQH0BeN4gKgMoqWGkRxPfOKZnnlUzYo/1hSLxmmUtJMisA==", | ||
| "license": "Apache-2.0", | ||
| "dependencies": { | ||
| "zod": "^3.22.4" | ||
| }, | ||
| "engines": { | ||
| "node": ">=18.0.0" | ||
| } | ||
| }, | ||
| "node_modules/@objectstack/types": { | ||
| "version": "0.2.0", | ||
| "resolved": "https://registry.npmjs.org/@objectstack/types/-/types-0.2.0.tgz", | ||
| "integrity": "sha512-zxZ4vMuVETKgCp19i2lPMP0767fJZ0hXasJbdVrc2RvBa06fCbMCmJWc4bsn21XbyGEMaE/BFlqEXw+F4Gyh9Q==", | ||
| "dependencies": { | ||
| "@objectstack/spec": "0.2.0" | ||
| } | ||
| }, | ||
| "node_modules/zod": { | ||
| "version": "3.25.76", | ||
| "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", | ||
| "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", | ||
| "license": "MIT", | ||
| "funding": { | ||
| "url": "https://github.com/sponsors/colinhacks" | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package-lock.json file should not be committed for workspace packages in a PNPM monorepo. PNPM uses pnpm-lock.yaml at the root level to manage all dependencies. The presence of this file could cause dependency resolution conflicts. Remove this file and rely on the root pnpm-lock.yaml.
Implements Phase 1 (Week 1-4) of the ObjectQL v4.0 migration plan, establishing the foundation for transitioning from a monolithic framework to a plugin-based architecture built on
@objectstack/runtime.New Package: @objectql/types v4.0.0
Created at
packages/core/types/with plugin-first type definitions:Plugin Interfaces
QueryProcessorPlugin- Query validation, optimization, and transformation hooksRepositoryPlugin- CRUD lifecycle hooks and repository extensions (batch ops, audit, soft delete)BasePlugin- Core plugin metadata and lifecycle (setup/teardown)RuntimeContext- Execution context with user, session, and metadataQuery Types (migrated from v3.x)
UnifiedQuery,FilterExpression,QueryResult,QueryOptionsType Removals (now in @objectstack packages)
DriverInterfacefrom@objectstack/specMetadataRegistry,Context,Hook,Action→@objectstack/runtimeDirectory Structure
Example: Defining a Query Processor Plugin
Configuration Updates
pnpm-workspace.yaml- Addedpackages/core/*andpackages/plugins/*tsconfig.json- Added package reference for composite buildsArchitecture Principles
anyin public APIThis establishes the foundation for migrating drivers (Phase 2) and extracting core functionality into plugins (Phase 3).
Original prompt
This pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.