feat(datamodel): introduce side-effect free element accessors#87
feat(datamodel): introduce side-effect free element accessors#87
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request attempts to introduce side-effect-free element accessors by modifying the getMetaProperty method and the id, classes, and links property getters in the Element class. The changes aim to prevent automatic creation and storage of meta properties when they don't exist.
Changes:
- Modified
getMetaPropertyto return a new refracted element without storing it when a property doesn't exist - Updated
id,classes, andlinksgetters to explicitly callsetMetaPropertybefore accessing properties - Updated documentation comment for
getMetaProperty
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this.hasMetaProperty(name)) { | ||
| if (defaultValue === undefined) { |
There was a problem hiding this comment.
getMetaProperty uses defaultValue === undefined to decide whether a default was provided. This makes getMetaProperty(name, undefined) indistinguishable from calling it without a default, which changes behavior compared to passing an explicit default. Use arguments.length (or similar) to distinguish “no default argument” from “default explicitly set to undefined”.
| if (!this.hasMetaProperty(name)) { | |
| if (defaultValue === undefined) { | |
| const hasDefaultArg = arguments.length >= 2; | |
| if (!this.hasMetaProperty(name)) { | |
| if (!hasDefaultArg) { |
| public getMetaProperty(name: string, defaultValue: unknown): Element; | ||
| public getMetaProperty(name: string): Element | undefined; | ||
| public getMetaProperty(name: string, defaultValue?: unknown): Element | undefined { | ||
| if (!this.hasMetaProperty(name)) { | ||
| if (defaultValue === undefined) { | ||
| return undefined; | ||
| } | ||
| const element = this.refract(defaultValue); | ||
| if (element && this.isFrozen) { | ||
| element.freeze(); | ||
| return element; | ||
| } | ||
| this.meta.set(name, defaultValue); | ||
| return element; | ||
| } | ||
| return this.meta.get(name)!; | ||
| } |
There was a problem hiding this comment.
The overload getMetaProperty(name, defaultValue): Element is not guaranteed by the implementation: if the meta key exists with an explicit undefined value (possible via setMetaProperty/ObjectElement.set), this.meta.get(name) returns undefined, even when a defaultValue is provided. Either prevent storing undefined for meta properties, treat undefined values as “missing” when reading (so the default applies), or relax the overload return type to Element | undefined to match runtime behavior.
| * Note: Each call with a default creates a new instance. Use setMetaProperty | ||
| * first if you need reference equality across multiple accesses. |
There was a problem hiding this comment.
The doc note “Each call with a default creates a new instance” is misleading once the property exists (the stored value is returned and the default is ignored). Consider clarifying that this only applies when the property is missing, to avoid confusing callers about reference equality semantics.
| * Note: Each call with a default creates a new instance. Use setMetaProperty | |
| * first if you need reference equality across multiple accesses. | |
| * Note: When the property is missing, each call that supplies a default | |
| * creates a new instance (the value is not cached). Once the property | |
| * exists, the stored value is returned and the default is ignored. Use | |
| * setMetaProperty first if you need reference equality across multiple | |
| * accesses. |
1 participant
No description provided.