Skip to content

Conversation

@jonasstrehle
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings November 16, 2025 22:33
Copilot finished reviewing on behalf of benStre November 16, 2025 22:37
Copy link

Copilot AI left a 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 introduces a new type interface system for DATEX, implementing a type registry that enables custom type bindings for JavaScript native types. The changes replace the previous hard-coded proxification approach with a more flexible, extensible architecture.

  • Implements a new TypeRegistry and TypeBinding system for managing type bindings
  • Adds type bindings for Map and Array collections with bidirectional synchronization
  • Refactors the DIFHandler to integrate type bindings and adds helper methods for triggering updates
  • Simplifies pointer type definitions by removing complex collection-specific types

Reviewed Changes

Copilot reviewed 17 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/dif/type-registry.ts New file implementing TypeRegistry and TypeBinding classes for managing type bindings
src/lib/js-core-types/map.ts New Map type binding with proxified methods for tracking changes
src/lib/js-core-types/array.ts New Array type binding with Proxy-based change tracking
src/lib/js-core-types/mod.ts Module index for js-core-types (only exports map.ts)
src/lib/special-core-types/endpoint.ts Moved Endpoint class to new location with WeakRef-based caching
src/dif/dif-handler.ts Integrated type registry, added trigger methods, removed hard-coded Map proxification
src/dif/definitions.ts Renamed DIFUpdateKind values (Push→Append, Remove→Delete) and restructured type definitions
src/dif/mod.ts Removed Builders export
src/dif/builders.ts Deleted file (functionality moved to DIFHandler trigger methods)
src/refs/ref.ts Updated to use new triggerReplace method
src/runtime/runtime.ts Simplified PointerOut type definitions, removed collection-specific types
src/runtime/mod.ts Removed special-core-types export
test/lib/js-core-types/map.test.ts New comprehensive test suite for Map type binding
test/lib/js-core-types/array.test.ts New comprehensive test suite for Array type binding
test/runtime/execute.test.ts Updated import path for Endpoint
test/runtime/dif.test.ts Removed redundant type assertion
rs-lib/Cargo.toml Changed datex-core dependency branch to feat/variant-access
deno.json Added datex-core-js/ import alias
Cargo.lock Updated dependencies (chumsky version, datex-core branch)
src/datex-core/datex_core_js.d.ts Reordered type definitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

});
const proxy = new Proxy(parent, {
set(target, prop, value, receiver) {
console.log("SET", prop, value);
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log statement left in production code. This will log on every array element assignment, which could clutter console output and impact performance.

Suggested change
console.log("SET", prop, value);

Copilot uses AI. Check for mistakes.
) {
const originalLength = array.length;
for (let i = from; i < to; i++) {
console.log(i, originalLength);
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug console.log statement left in production code. This will log during array fill operations.

Suggested change
console.log(i, originalLength);

Copilot uses AI. Check for mistakes.
@@ -0,0 +1 @@
export * from "./map.ts";
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mod.ts file only exports map.ts but not array.ts. This means arrayTypeBinding cannot be imported from the module index. Users would need to import directly from ./array.ts, which is inconsistent with the module pattern.

Add the array export:

export * from "./map.ts";
export * from "./array.ts";
Suggested change
export * from "./map.ts";
export * from "./map.ts";
export * from "./array.ts";

Copilot uses AI. Check for mistakes.
Comment on lines +101 to +161
const typeBinding = this.#typeBindings.get(typePointerAddress);
if (typeBinding) {
return typeBinding;
} else {
return null;
}
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The conditional logic can be simplified by removing the redundant else branch and directly returning the result.

public getTypeBinding(
    typePointerAddress: string,
): TypeBinding<unknown> | null {
    return this.#typeBindings.get(typePointerAddress) ?? null;
}
Suggested change
const typeBinding = this.#typeBindings.get(typePointerAddress);
if (typeBinding) {
return typeBinding;
} else {
return null;
}
return this.#typeBindings.get(typePointerAddress) ?? null;

Copilot uses AI. Check for mistakes.
export type TypeDefinition = {
name: string;
structuralDefinition: DIFTypeContainer; // TODO: generic definition
ownImpls?: OwnImpl[]; // e.g. impl CustomMapMap
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the comment: "impl CustomMapMap" should likely be "impl CustomMap" (duplicate "Map" at the end).

Suggested change
ownImpls?: OwnImpl[]; // e.g. impl CustomMapMap
ownImpls?: OwnImpl[]; // e.g. impl CustomMap

Copilot uses AI. Check for mistakes.
clear.call(parent);
},
handleReplace(parent, newValue: unknown) {
// TODO: replace all map entries
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handleReplace implementation is incomplete (marked with TODO). This means that when a map receives a 'replace' update from remote endpoints, it won't properly update its entries. This could lead to inconsistent state across endpoints.

Consider implementing this by clearing the map and repopulating it:

handleReplace(parent, newValue: unknown) {
    const clear = parent[ORIGINAL_CLEAR];
    clear.call(parent);
    if (newValue instanceof Map) {
        const set = parent[ORIGINAL_SET];
        for (const [key, val] of newValue.entries()) {
            set.call(parent, key, val);
        }
    }
}
Suggested change
// TODO: replace all map entries
const clear = parent[ORIGINAL_CLEAR];
clear.call(parent);
if (newValue instanceof Map) {
const set = parent[ORIGINAL_SET];
for (const [key, val] of newValue.entries()) {
set.call(parent, key, val);
}
}

Copilot uses AI. Check for mistakes.
difHandler: DIFHandler,
) {
for (let i = 0; i < deleteCount; i++) {
difHandler.triggerDelete(pointerAddress, start + i);
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The triggerArraySplice function has a logic issue: when deleting multiple consecutive elements, it repeatedly deletes at start + i, but after the first deletion, the indices shift. This means it will delete the wrong elements.

For example, deleting 3 elements starting at index 2:

  • First iteration: deletes index 2 (correct)
  • Second iteration: deletes index 3, but the original index 3 has shifted to index 2
  • Third iteration: deletes index 4, but the original index 4 has shifted to index 2

The function should delete at the same index repeatedly:

for (let i = 0; i < deleteCount; i++) {
    difHandler.triggerDelete(pointerAddress, start);
}
Suggested change
difHandler.triggerDelete(pointerAddress, start + i);
difHandler.triggerDelete(pointerAddress, start);

Copilot uses AI. Check for mistakes.
// MyTrait.autoSelectFamily(x);
// Type.autoSelectFamily(x);
// object->contains()
// obj.hasOwnPropety()
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the commented code: "hasOwnPropety" should be "hasOwnProperty".

Suggested change
// obj.hasOwnPropety()
// obj.hasOwnProperty()

Copilot uses AI. Check for mistakes.
});
assertEquals(map.get("externalKey"), "newValue");

// DELETE (no effect since map is already empty)
Copy link

Copilot AI Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test comment is misleading: it says "DELETE (no effect since map is already empty)" but at this point the map is not empty - it still contains "externalKey" that was just added in the previous test step. The map only becomes empty after the DELETE operation.

Suggested change
// DELETE (no effect since map is already empty)
// DELETE (removes "externalKey", map becomes empty)

Copilot uses AI. Check for mistakes.
@benStre benStre force-pushed the feat/type-interfaces branch from 642e7dc to 5f1fe9a Compare November 24, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants