-
Notifications
You must be signed in to change notification settings - Fork 0
Migrate @objectql/types to use @objectstack/spec as protocol constitution #135
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
base: main
Are you sure you want to change the base?
Conversation
…ql/types Co-authored-by: hotlong <[email protected]>
…minLength, maxLength) Co-authored-by: hotlong <[email protected]>
Co-authored-by: hotlong <[email protected]>
|
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 PR migrates the type system to use @objectstack/spec v0.1.1 as the single source of truth for protocol definitions, eliminating duplicate type definitions and establishing a clean separation between protocol (wire format) and runtime (implementation) types.
Changes:
- Refactored
@objectql/typesto import and extend protocol types from@objectstack/specrather than duplicating definitions - Standardized field type and property naming to match protocol conventions (
auto_number→autonumber,max_length→maxLength) - Updated core utilities, GraphQL adapter, metadata endpoints, and CLI tools to use protocol-compliant naming
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/foundation/types/package.json | Added @objectstack/spec dependency and temporarily disabled JSON schema generation |
| packages/foundation/types/src/index.ts | Added documentation explaining the protocol/runtime type relationship |
| packages/foundation/types/src/field.ts | Refactored to import protocol types and extend with runtime-specific properties |
| packages/foundation/types/src/object.ts | Refactored to import ServiceObject from spec and extend with runtime features |
| packages/foundation/types/src/action.ts | Refactored to import Action from spec and extend with runtime execution capabilities |
| packages/foundation/core/src/util.ts | Updated to use protocol-compliant property names (maxLength) |
| packages/foundation/core/test/util.test.ts | Updated test assertions to match new property names |
| packages/runtime/server/src/metadata.ts | Updated metadata serialization to use protocol-compliant names |
| packages/runtime/server/src/adapters/graphql.ts | Updated field type mapping from auto_number to autonumber |
| packages/tools/cli/src/commands/generate.ts | Updated TypeScript mapping from auto_number to autonumber |
| packages/tools/cli/src/commands/sync.ts | Updated database introspection to use maxLength property |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| description?: string; | ||
|
|
||
| /** The data type of the field. */ | ||
| export interface FieldConfig extends Omit<Field, 'type' | 'options' | 'required' | 'multiple' | 'unique' | 'deleteBehavior' | 'hidden' | 'readonly' | 'encryption' | 'index' | 'externalId'> { |
Copilot
AI
Jan 18, 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 Omit list contains 11 properties being excluded from the base Field type. This creates a tight coupling to the protocol definition and makes the interface fragile - any changes to the protocol's Field interface could silently break this contract. Consider using a Pick approach to explicitly declare which protocol properties are inherited, or document why each property needs to be re-declared.
| /** | ||
| * Reference to another object for lookup/master_detail fields. | ||
| * Specifies the target object name for relationship fields. | ||
| * @deprecated Use 'reference' from SpecField instead |
Copilot
AI
Jan 18, 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.
This property is marked as deprecated in favor of the protocol's reference field, but there's no migration path or guidance provided. Consider adding a comment explaining how to migrate from reference_to to reference, or implement a getter/setter that maps between the two to ease the transition.
| * @deprecated Use 'reference' from SpecField instead | |
| * | |
| * @deprecated Legacy alias for the protocol-level {@link SpecField.reference} field. | |
| * | |
| * Migration guidance: | |
| * - New schemas MUST use the `reference` field defined on SpecField (wire protocol) instead of `reference_to`. | |
| * - During migration, engines and tooling SHOULD: | |
| * - Treat `reference_to` as a fallback alias for `reference` (i.e., if `reference` is undefined and `reference_to` | |
| * is defined, behave as though `reference` were set to the same value). | |
| * - Prefer the protocol `reference` value when both are present. | |
| * | |
| * This property is retained only for backward compatibility with older *.object.yml definitions and may be removed | |
| * in a future major version once all callers have migrated to `reference`. |
| * Regular expression pattern for validation. | ||
| * @deprecated Use validation.pattern instead | ||
| */ | ||
| regex?: string; |
Copilot
AI
Jan 18, 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.
This property is marked as deprecated in favor of validation.pattern, but it's also included again inside the validation object (line 230 has regex as an alias for pattern). This creates ambiguity about which property should be used. Consider removing the deprecated top-level property entirely or providing a clear migration timeline.
| if (foreignKey) { | ||
| // This is a lookup field | ||
| fieldConfig = { | ||
| name: column.name, |
Copilot
AI
Jan 18, 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 name property is now explicitly set in both lookup and regular field configurations. This is redundant since the comment on line 114 in the original code states 'If defined within an object map, this is often automatically populated from the key.' Consider documenting why explicit name assignment is now required, or if this is needed for protocol compliance.
| const fieldType = mapDatabaseTypeToFieldType(column.type); | ||
|
|
||
| fieldConfig = { | ||
| name: column.name, |
Copilot
AI
Jan 18, 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 name property is now explicitly set in both lookup and regular field configurations. This is redundant since the comment on line 114 in the original code states 'If defined within an object map, this is often automatically populated from the key.' Consider documenting why explicit name assignment is now required, or if this is needed for protocol compliance.
Refactors the type system to use
@objectstack/specv0.1.1 as the single source of truth for all metadata/AST interfaces, eliminating duplicate type definitions and establishing a clean protocol/runtime separation.Changes
Type System Architecture
Field,ServiceObject,Action) from@objectstack/specFieldConfigfor ergonomic runtime usageProtocol Alignment
auto_number→autonumbermin_length/max_length→minLength/maxLengthExample
Before:
After:
Build Configuration
Architecture
Protocol types remain stable and versioned upstream. Runtime features extend without polluting the wire format.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
fastdl.mongodb.org/usr/local/bin/node /usr/local/bin/node /home/REDACTED/work/objectql/objectql/node_modules/.pnpm/[email protected]/node_modules/jest-worker/build/processChild.js ndor/bin/sh grep n/je�� ' --include '*.yml' --exclude '*-o export type Field wcase/project-tracker/node_modules/.bin/node --filter @objectwatchman --global enerator.js iptables modu�� --production security k/objectql/objectql/node_modules/.bin/sh OUTPUT -d 168.63.129.16 sh(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
@objectstack/spec(v0.1.1) is now the "Constitution" for all Metadata/AST interfaces. #134💡 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.