wip/term wrapper type#74
Conversation
jeswr
commented
Apr 20, 2026
jeswr
commented
- feat: introduce from method
- chore: update docs
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TermWrapper.from(...) factory method intended to improve typing when creating wrappers (so instances can be used directly where RDF/JS Term types are expected), and updates documentation/examples accordingly. It also removes the IRdfJsTerm interface and adjusts internal mappers/helpers to work with the updated TermWrapper shape.
Changes:
- Add
TermWrapper.from(...)with overloads to return an intersection of the wrapper instance and the underlying RDF/JS term type. - Remove
IRdfJsTermand update internal mapping/ensure helpers to useTermWrapper. - Update README and in-code docs to use
.from(...)instead ofnew.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/type/IRdfJsTerm.ts | Removes the old interface that declared the “all term members” surface area. |
| src/TermWrapper.ts | Adds .from(...), makes TermWrapper generic over the wrapped term type, and moves “extra term members” to runtime-defined properties. |
| src/mapping/TermFrom.ts | Updates instance(...) mapper to accept TermWrapper. |
| src/mapping/LiteralAs.ts | Adds casts to Literal to access language/datatype after the IRdfJsTerm removal. |
| src/ensure.ts | Updates ensureDatatype/ensureListRoot to accept TermWrapper and uses Literal casts for datatype checks. |
| README.md | Updates examples to use Person.from(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static from<This extends new (...args: any) => any>( | ||
| this: This, | ||
| term: string, | ||
| dataset: DatasetCore, | ||
| factory: DataFactory, | ||
| ): InstanceType<This> & NamedNode<string> | ||
| public static from<This extends new (...args: any) => any, T extends Term>( | ||
| this: This, | ||
| term: T, | ||
| dataset: DatasetCore, | ||
| factory: DataFactory, | ||
| ): InstanceType<This> & T | ||
| public static from(this: any, term: string | Term, dataset: DatasetCore, factory: DataFactory): any { | ||
| return new this(term, dataset, factory) | ||
| } |
There was a problem hiding this comment.
TermWrapper.from is typed with This extends new (...args: any) => any, which makes the method callable on subclasses whose constructors don’t match (term, dataset, factory) (e.g. ListItem, Overwriter). That will type-check but throw at runtime due to missing constructor args. Tighten the this constructor constraint to the expected signature (or change from to forward ...args: ConstructorParameters<This>), so invalid .from(...) calls are rejected at compile time.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Implemented in 70b01fc. TermWrapper.from now constrains this to constructors matching (term, dataset, factory), so .from(...) is no longer type-valid for subclasses with incompatible constructor signatures.
| public static from<This extends new (...args: any) => any>( | ||
| this: This, | ||
| term: string, | ||
| dataset: DatasetCore, | ||
| factory: DataFactory, | ||
| ): InstanceType<This> & NamedNode<string> | ||
| public static from<This extends new (...args: any) => any, T extends Term>( | ||
| this: This, | ||
| term: T, | ||
| dataset: DatasetCore, | ||
| factory: DataFactory, | ||
| ): InstanceType<This> & T | ||
| public static from(this: any, term: string | Term, dataset: DatasetCore, factory: DataFactory): any { | ||
| return new this(term, dataset, factory) | ||
| } |
There was a problem hiding this comment.
New TermWrapper.from(...) is a public entrypoint but there’s no unit test coverage asserting it returns a working instance (and preserves dataset/factory) for both term: string and term: Term overloads. Adding a focused test in the existing term-wrapper unit tests would help prevent regressions in the typing/constructor behavior.
| ensureDatatype(term, ...byteArrayDatatypes) | ||
|
|
||
| switch (term.datatype.value) { | ||
| switch ((term as unknown as Literal).datatype.value) { |
There was a problem hiding this comment.
uInt8Array uses (term as unknown as Literal).datatype.value after the term-type check. If TermWrapper re-declares datatype in its TS shape (or ensureTermType becomes an asserts-style type guard), this cast can be removed and the mapper stays type-safe.
| ensureDatatype(term, RDF.langString) | ||
|
|
||
| return [term.language, term.value] | ||
| return [(term as unknown as Literal).language, term.value] |
There was a problem hiding this comment.
langTuple is using term as unknown as Literal just to access .language. To keep this mapper clean and type-safe, consider restoring language on TermWrapper’s declared type (or returning a narrowed literal type from ensureTermType) so this doesn’t require an unsafe cast.
| export namespace TermFrom { | ||
| export function instance(value: IRdfJsTerm, factory: DataFactory): Term { | ||
| export function instance(value: TermWrapper, factory: DataFactory): Term { | ||
| return itself(value as Term, factory) |
There was a problem hiding this comment.
TermFrom.instance casts value as Term, but TermWrapper is already structurally compatible with Term (it exposes termType, value, and equals). Dropping the cast and passing value directly would keep this mapper simpler and avoid masking future typing issues.
| return itself(value as Term, factory) | |
| return itself(value, factory) |
| for (const prop of ['language', 'direction', 'datatype', 'subject', 'predicate', 'object', 'graph'] as const) { | ||
| Object.defineProperty(TermWrapper.prototype, prop, { | ||
| get(this: TermWrapper) { return (this as any).original[prop] }, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
These RDF/JS-term properties are being added at runtime via Object.defineProperty, but the class doesn’t declare them in its TypeScript shape anymore. That forces downstream code to use as unknown as Literal casts and is a public typing regression vs the previous IRdfJsTerm interface. Consider adding declare readonly language/direction/datatype/subject/predicate/object/graph (or a replacement interface) so consumers and internal mappers can access them without unsafe casts while keeping the runtime implementation as-is.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/rdfjs/wrapper/sessions/4e35594d-9a08-4e2a-8527-5235d06363cc Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
