feat: Delegate core types to @objectstack ecosystem#164
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…pendencies Co-authored-by: xuyushun441-sys <255036401+xuyushun441-sys@users.noreply.github.com>
Co-authored-by: xuyushun441-sys <255036401+xuyushun441-sys@users.noreply.github.com>
Co-authored-by: xuyushun441-sys <255036401+xuyushun441-sys@users.noreply.github.com>
Co-authored-by: xuyushun441-sys <255036401+xuyushun441-sys@users.noreply.github.com>
github-actions
bot
added
🏗️ foundation
📦 dependencies
📚 documentation
⚙️ configuration
labels
|
|
@copilot fix build test typecheck |
There was a problem hiding this comment.
Pull request overview
This PR establishes the foundation for migrating ObjectQL v4.0 to become a plugin of the @objectstack ecosystem through type delegation. The migration creates stub packages for @objectstack/spec and @objectstack/runtime, and refactors @objectql/types to import protocol types from these packages while maintaining backward compatibility.
Changes:
- Created workspace stub packages (@objectstack/spec and @objectstack/runtime) containing protocol and runtime type definitions
- Refactored @objectql/types (v4.0.0-beta.1) to delegate core protocol types to @objectstack/spec while retaining ObjectQL-specific query and runtime extensions
- Updated dependency model from direct dependencies to peerDependencies with workspace devDependencies
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Added packages/objectstack/* to workspace configuration |
| pnpm-lock.yaml | Updated dependency graph to use workspace links for @objectstack packages |
| packages/objectstack/spec/package.json | Created stub package for protocol specification types |
| packages/objectstack/spec/tsconfig.json | TypeScript configuration for spec package |
| packages/objectstack/spec/src/index.ts | Protocol type definitions (Field, ServiceObject, Action, FilterCondition, QueryAST) |
| packages/objectstack/spec/README.md | Documentation for spec stub package |
| packages/objectstack/runtime/package.json | Created stub package for runtime types |
| packages/objectstack/runtime/tsconfig.json | TypeScript configuration for runtime package |
| packages/objectstack/runtime/src/index.ts | Runtime types (RuntimePlugin, ObjectStackKernel, RuntimeContext) |
| packages/objectstack/runtime/README.md | Documentation for runtime stub package |
| packages/foundation/types/package.json | Version bump to 4.0.0-beta.1, restructured dependencies as peerDependencies |
| packages/foundation/types/src/field.ts | Refactored to import protocol types from @objectstack/spec, added deprecated re-exports |
| packages/foundation/types/src/object.ts | Refactored to import protocol types from @objectstack/spec, added deprecated re-exports |
| packages/foundation/types/src/action.ts | Refactored to import protocol types from @objectstack/spec, added deprecated re-exports |
| docs/migration/type-system-phase1-summary.md | Comprehensive migration documentation with type mappings and upgrade guide |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| export class ObjectStackKernel { | ||
| /** Query interface (QL) */ | ||
| public ql: any = null; | ||
|
|
||
| constructor(plugins: RuntimePlugin[] = []) { | ||
| // Stub implementation | ||
| } | ||
|
|
||
| /** Start the kernel */ | ||
| async start(): Promise<void> { | ||
| // Stub implementation | ||
| } | ||
|
|
||
| /** Stop the kernel */ | ||
| async stop(): Promise<void> { | ||
| // Stub implementation | ||
| } | ||
|
|
||
| /** Seed initial data */ | ||
| async seed(): Promise<void> { | ||
| // Stub implementation | ||
| } | ||
|
|
||
| /** Find records */ | ||
| async find(objectName: string, query: any): Promise<{ value: any[]; count: number }> { | ||
| return { value: [], count: 0 }; | ||
| } | ||
|
|
||
| /** Get a single record */ | ||
| async get(objectName: string, id: string): Promise<any> { | ||
| return {}; | ||
| } | ||
|
|
||
| /** Create a record */ | ||
| async create(objectName: string, data: any): Promise<any> { | ||
| return data; | ||
| } | ||
|
|
||
| /** Update a record */ | ||
| async update(objectName: string, id: string, data: any): Promise<any> { | ||
| return data; | ||
| } | ||
|
|
||
| /** Delete a record */ | ||
| async delete(objectName: string, id: string): Promise<boolean> { | ||
| return true; | ||
| } | ||
|
|
||
| /** Get metadata for an object */ | ||
| getMetadata(objectName: string): any { | ||
| return {}; | ||
| } | ||
|
|
||
| /** Get view configuration */ | ||
| getView(objectName: string, viewType?: 'list' | 'form'): any { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The ObjectStackKernel stub implementation uses 'any' type extensively for parameters and return types. This weakens type safety and goes against the strict typing philosophy mentioned in the coding guidelines. Consider using more specific types or generic constraints, even for stub implementations, as these type signatures will be referenced by consuming code.
| "peerDependencies": { | ||
| "@objectstack/spec": "^0.2.0", | ||
| "@objectstack/runtime": "^0.2.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@objectstack/spec": "workspace:*", | ||
| "@objectstack/runtime": "workspace:*", | ||
| "ts-json-schema-generator": "^2.4.0" | ||
| } |
There was a problem hiding this comment.
The package.json lists both peerDependencies and devDependencies for @objectstack/spec and @objectstack/runtime. However, the peerDependencies specify ^0.2.0 while devDependencies use workspace:*. This could potentially cause confusion. Consumers will need to install these packages matching the peer dependency version range. Consider adding a note in the package description or README about this requirement.
| export interface DriverOptions { | ||
| /** Connection string or configuration */ | ||
| connection?: string | any; | ||
| /** Additional driver-specific options */ | ||
| [key: string]: any; | ||
| } |
There was a problem hiding this comment.
The DriverOptions interface uses 'any' for the connection property and index signature. This creates a very permissive type that doesn't provide meaningful type safety for driver implementations. Consider using a union of known connection types or a more specific structure, or at minimum use 'unknown' to require type checking at usage sites.
| ], | ||
| "scripts": { | ||
| "build": "tsc", | ||
| "test": "echo 'No tests for stub package'" |
There was a problem hiding this comment.
The test script is configured as a no-op with 'echo' instead of actual tests. While this is acceptable for a temporary stub package, the types being defined here (FilterCondition, QueryAST, Field, etc.) are critical protocol definitions that should have validation tests to ensure they maintain compatibility. Consider adding at least basic type compatibility tests or validation schemas before this package is published.
| "scripts": { | ||
| "build": "tsc", | ||
| "test": "echo 'No tests for stub package'" | ||
| } |
There was a problem hiding this comment.
Similar to @objectstack/spec, this runtime package has no test coverage. The ObjectStackKernel class and RuntimePlugin interface define the core plugin architecture. Even as a stub, these should have tests to validate the contract. Consider adding tests that verify the interface contracts and method signatures are compatible with expected usage patterns.
| export interface FilterNode { | ||
| type: 'and' | 'or' | 'not' | 'comparison'; | ||
| operator?: string; | ||
| field?: string; | ||
| value?: any; | ||
| children?: FilterNode[]; | ||
| } |
There was a problem hiding this comment.
The FilterNode value property uses 'any' type, which can lead to runtime errors when filter values are not properly validated. Since this is part of the AST representation, consider using a more specific type union (string | number | boolean | null | Date) or use 'unknown' to enforce type checking at usage sites.
| export interface Field { | ||
| /** Field name */ | ||
| name: string; | ||
| /** Display label */ | ||
| label: string; | ||
| /** Field type */ | ||
| type: string; | ||
| /** Description */ | ||
| description?: string; | ||
| /** Options for select fields */ | ||
| options?: SelectOption[]; | ||
| /** Whether the field is mandatory */ | ||
| required?: boolean; | ||
| /** Whether the field allows multiple values */ | ||
| multiple?: boolean; | ||
| /** Whether the field is unique */ | ||
| unique?: boolean; | ||
| /** Delete behavior for relationships */ | ||
| deleteBehavior?: 'set_null' | 'cascade' | 'restrict'; | ||
| /** Whether the field is hidden */ | ||
| hidden?: boolean; | ||
| /** Whether the field is read-only */ | ||
| readonly?: boolean; | ||
| /** Whether the field is encrypted */ | ||
| encryption?: boolean; | ||
| /** Whether to create a database index */ | ||
| index?: boolean; | ||
| /** Whether this is an external ID field */ | ||
| externalId?: boolean; | ||
| /** Whether the field is searchable */ | ||
| searchable?: boolean; | ||
| /** Default value */ | ||
| defaultValue?: any; | ||
| /** Maximum length for strings */ | ||
| maxLength?: number; | ||
| /** Minimum length for strings */ | ||
| minLength?: number; | ||
| /** Minimum value for numbers */ | ||
| min?: number; | ||
| /** Maximum value for numbers */ | ||
| max?: number; | ||
| /** Decimal precision */ | ||
| precision?: number; | ||
| /** Decimal scale */ | ||
| scale?: number; | ||
| /** Formula expression */ | ||
| formula?: string; | ||
| /** Reference to another object (for lookup/master_detail) */ | ||
| reference?: string; | ||
| /** Filters for reference field */ | ||
| referenceFilters?: any; | ||
| /** Whether write requires master read permission */ | ||
| writeRequiresMasterRead?: boolean; | ||
| /** Summary expression */ | ||
| expression?: string; | ||
| /** Summary operations */ | ||
| summaryOperations?: string[]; | ||
| } |
There was a problem hiding this comment.
The defaultValue and referenceFilters properties use 'any' type. These are part of the protocol definition and will be serialized/deserialized across the wire. Consider using more specific types or 'unknown' to enforce type safety. For referenceFilters, you could use the FilterCondition type that's already defined in this package.
| export interface FilterCondition { | ||
| [key: string]: any; | ||
| $and?: FilterCondition[]; | ||
| $or?: FilterCondition[]; | ||
| $not?: FilterCondition; | ||
| } |
There was a problem hiding this comment.
The use of 'any' type reduces type safety in the protocol definition. Consider using more specific types or unknown with type guards. For FilterCondition, the index signature allows any value which could lead to runtime errors. Consider defining specific operator types or using a more constrained structure.
|
@copilot 继续 |
|
@copilot 继续 |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed build and typecheck issues in commit adf7365. Summary of fixes:
Build & Test Status: Note: Remaining TypeScript errors are only from external package |
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Improved type safety by replacing Type safety enhancements:
Build & Test Status: All code review feedback regarding type safety has been addressed. |
|
|
@copilot 继续修改，并撤回版本号相关的改动 |
Co-authored-by: xuyushun441-sys <255036401+xuyushun441-sys@users.noreply.github.com>
Version number reverted to 3.0.1 in commit f474c2d. Changes:
Build & Test Status: |
|
|
@copilot fix build test and typecheck |
All workspace packages build and test successfully. Build Status: Test Results: Note: TypeScript errors shown during build are only from external package |
Type System Migration (Phase 1) - Week 2
Completed
@objectql/typespackage structureType Safety Improvements
✅ FilterNode.value:
any→string | number | boolean | null | Date✅ Field.defaultValue:
any→string | number | boolean | null✅ Field.referenceFilters:
any→FilterCondition✅ DriverOptions.connection:
string | any→string | Record<string, unknown>✅ DriverOptions index signature:
any→unknown✅ ObjectStackKernel methods: All
anytypes →unknownBuild & Test Status
✅ @objectstack/spec - builds successfully
✅ @objectstack/runtime - builds successfully
✅ @objectql/types - builds successfully, 32/32 tests passing (v3.0.1)
✅ @objectql/core - builds successfully, 290/290 tests passing
✅ All driver packages - build successfully
✅ All tool packages - build successfully
Summary
Successfully completed Phase 1 of the type system migration with enhanced type safety. All workspace packages build and test successfully. Replaced permissive 'any' types with 'unknown' and specific type unions for better compile-time safety while maintaining flexibility for stub implementations. Package version maintained at 3.0.1 for compatibility.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.