Skip to content

Introduce TermWrapper#from and TermWrapper#in#61

Open
jeswr wants to merge 12 commits intomainfrom
feat/from-in
Open

Introduce TermWrapper#from and TermWrapper#in#61
jeswr wants to merge 12 commits intomainfrom
feat/from-in

Conversation

@jeswr
Copy link
Copy Markdown
Collaborator

@jeswr jeswr commented Apr 13, 2026

These are static methods which replace the need to use the new constructor. They are able to return conjunction types and have overloads.

This means that the return value of the function is always Wrapper & T where Wrapper is the object extending TermWrapper and T extends Term and is inferred from the input term.

Breaking type changes are made in this PR. Specifically, the #subjectPredicate now methods require an input type of TermWrapper & Term to enable internal casting to be removed. Previously the narrower type TermWrapper was required and casting was used internally. These breaking changes are introduced to make the internal codebase cleaner following the introduction of the new APIs -- they are not required to implement the new #from and #in API's.

Copilot AI review requested due to automatic review settings April 13, 2026 01:14
Comment thread src/TermWrapper.ts Fixed
Copy link
Copy Markdown
Contributor

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 static factory methods (TermWrapper.from and TermWrapper.in) to avoid direct constructor usage, enabling better inference of the wrapped RDF/JS term type (returning Wrapper & T), and updates the mapping API surface to work with the new TermNode pattern via wrapper.node.

Changes:

  • Add TermWrapper.from(...) and TermWrapper.in(...) factories and a TermNode type to preserve term-specific typing (e.g., Literal.language, Quad.subject) through wrappers.
  • Refactor mapping utilities (RequiredFrom, OptionalFrom, SetFrom, Mapping, WrappingMap/Set, etc.) to accept TermNode and update call sites to pass this.node.
  • Add/update tests to assert type-level visibility of term-specific properties and update usage away from new.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/unit/wrapping_map.test.ts Switches wrapper instantiation to .from and mapping calls to use this.node.
test/unit/term_wrapper.test.ts Updates wrapper creation to .from and adjusts recursion assertions.
test/unit/term_wrapper_properties.test.ts New tests validating term-specific property visibility at type/runtime.
test/unit/rdf_list.test.ts Switches wrapper instantiation to .from and mapping calls to use this.node.
test/unit/model/Parent.ts Updates mappings to use this.node; changes recursion property typing.
test/unit/model/Child.ts Updates mappings to use this.node.
test/unit/literalAs.test.ts Updates to use TermWrapper.from(...) instead of new TermWrapper(...).
test/unit/decorators.test.ts Updates decorated wrapper instantiation to .from.
test/unit/dataset_wrapper_types.test.ts New tests asserting DatasetWrapper-produced wrapper types.
src/WrappingSet.ts Uses TermNode anchor and .from when wrapping matched objects.
src/WrappingMap.ts Uses TermNode anchor and .from when wrapping matched objects.
src/type/ITermWrapperConstructor.ts Extends constructor type to include static .from.
src/type/ITermAsValueMapping.ts Changes mapper input from TermWrapper to TermNode.
src/TermWrapper.ts Introduces TermNode, factories (from, in), and delegated term-specific getters.
src/RdfList.ts Updates subject typing to TermNode; adds RdfList.from(...) helper.
src/Overwriter.ts Updates subject typing to TermNode and mapping calls to use this.node.
src/mod.ts Removes export of IAnyTerm type.
src/mapping/TermFrom.ts Narrows TermFrom.instance input type to TermWrapper.
src/mapping/TermAs.ts Updates instance/list helpers to work with TermNode and .from.
src/mapping/SetFrom.ts Updates anchor type to TermNode.
src/mapping/RequiredFrom.ts Updates anchor type to TermNode and uses .from for wrapping.
src/mapping/RequiredAs.ts Updates anchor type to TermNode.
src/mapping/OptionalFrom.ts Updates anchor type to TermNode and uses .from for wrapping.
src/mapping/OptionalAs.ts Updates anchor type to TermNode and removes as Term casts in matches.
src/mapping/NamedNodeAs.ts Updates mapper input type to TermNode.
src/mapping/Mapping.ts Updates anchor type to TermNode.
src/mapping/LiteralAs.ts Updates mapper input type to TermNode and switches term-type error path.
src/ListItem.ts Updates mapping calls to use this.node and adjusts rest-mapper parameter type.
src/errors/TermTypeError.ts Updates docs/examples to use this.node.
src/errors/LiteralDatatypeError.ts Updates docs/examples to use this.node.
src/ensure.ts Replaces IAnyTerm usages with RDF/JS Term/Literal types.
src/decorators/setter.ts Updates decorator implementation to pass this.node into mapping functions.
src/decorators/getter.ts Updates decorator implementation to pass this.node into mapping functions.
src/DatasetWrapper.ts Uses .from for wrapper construction and returns dataset iterator directly.
README.md Updates examples to use .from(...) and this.node in mappings.

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

Comment thread src/TermWrapper.ts Outdated
Comment thread src/TermWrapper.ts Outdated
Comment thread src/mapping/LiteralAs.ts Outdated
Comment thread src/type/ITermWrapperConstructor.ts Outdated
Comment thread test/unit/model/Parent.ts
@jeswr
Copy link
Copy Markdown
Collaborator Author

jeswr commented Apr 13, 2026

@copilot apply changes based on the comments in this thread

… fix hasRecursive getter

Agent-Logs-Url: https://github.com/rdfjs/wrapper/sessions/4d9402d1-0701-4aa8-8ff6-a2bab07555bb

Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 13, 2026

@copilot apply changes based on the comments in this thread

Applied all changes from the review thread in commit 98b1fbf:

  • src/TermWrapper.ts: Removed the unused ./mod.js import (circular dep) and changed enumerable: trueenumerable: false on the Object.defineProperty loop.
  • src/mapping/LiteralAs.ts: Changed TermTypeError import to come directly from ../errors/TermTypeError.js instead of ../mod.js, and removed unused ensureTermType and Term imports.
  • src/type/ITermWrapperConstructor.ts: Changed TermWrapper import to ../TermWrapper.js instead of ../mod.js.
  • test/unit/model/Parent.ts: Changed hasRecursive getter to use OptionalFrom.subjectPredicate (matching the Parent | undefined return type and setter accepting undefined).

@langsamu langsamu mentioned this pull request Apr 13, 2026
@jeswr
Copy link
Copy Markdown
Collaborator Author

jeswr commented Apr 13, 2026

@copilot resolve the merge conflicts in this pull request

Copilot AI and others added 2 commits April 13, 2026 20:40
Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 13, 2026

@copilot resolve the merge conflicts in this pull request

Resolved all merge conflicts in commit a49c9de. The merge integrates:

  • src/TermWrapper.ts: Merged our generics, from/in/node additions with main's JSDoc docs, IRdfJsTerm interface, and constructor overloads. Added declare statements for dynamically-added properties to satisfy implements IRdfJsTerm.
  • src/ensure.ts: Adopted IRdfJsTerm parameter types from main.
  • src/mapping/TermFrom.ts: Adopted IRdfJsTerm type from main.
  • src/mod.ts: Removed decorator exports (deleted on main).
  • Deleted src/decorators/getter.ts, src/decorators/setter.ts, test/unit/decorators.test.ts (removed on main).
  • src/mapping/NamedNodeAs.ts: Updated new url function to use TermNode consistently.

Also fixed the recursive test in 4b8023b to match the OptionalFrom behavior (asserts undefined instead of expecting a throw). All 150 tests pass.

Comment thread src/mapping/TermAs.ts Outdated
@matthieubosquet
Copy link
Copy Markdown
Collaborator

This PR is bringing a lot of complexity to types that were intentionally designed to be easily read and understood by users.

Additionally, one of the main advantages I see in this kind of type safety would be manually wrapping terms. This library is not to designed to wrap terms out of context (i.e. this is not the intended paradigm). Rather, TermWrapper is intended to be a base class for object oriented specialisations of Term. There is no practical advantage of the type safety given by this change when used in the context of mapping.

Baring the cost of moving away from easily comprehensible idiomatic typings for the sake of type safety that would practically never be used is not a good idea.

Does that make sense to you @jeswr ?

@jeswr jeswr mentioned this pull request Apr 19, 2026
@jeswr
Copy link
Copy Markdown
Collaborator Author

jeswr commented Apr 19, 2026

@copilot resolve the merge conflicts in this pull request

Co-authored-by: jeswr <63333554+jeswr@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 19, 2026

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts with origin/main in ae77026:

  • src/DatasetWrapper.ts: Combined main's new named() method (with Quad_Graph overloads, NamedGraphDataset view) with our generic constraints (T extends TermWrapper) and conjunction return types (Iterable<T & Quad_Subject>, Iterable<T & Quad_Object>). Adopted main's iterator style. Kept termWrapper.from(...) (instead of new termWrapper(...)) since this PR replaces direct constructor usage.
  • src/mapping/LiteralAs.ts: For the new url() function, used XSD.anyURI (renamed on main) with the inline termType check pattern that the rest of this file uses on this branch.

npx tsc (src) passes cleanly and all 169 runtime tests pass.

@jeswr
Copy link
Copy Markdown
Collaborator Author

jeswr commented Apr 19, 2026

This PR is bringing a lot of complexity to types that were intentionally designed to be easily read and understood by users.

I disagree with the assumption. Whilst there is some additional complexity to the types that maintainers face in looking at TermWrapper, the consequence is an easier to understand set of typings for the consumer of the library.

For instance, my code editor can pickup that language is not available when I am wrapping a NamedNode.

image image

If a generic term is input, you get back this kind of behavior, where you are enouraged to do a type check before using properties that are only available for some terms

image

Additionally, one of the main advantages I see in this kind of type safety would be manually wrapping terms. This library is not to designed to wrap terms out of context (i.e. this is not the intended paradigm). Rather, TermWrapper is intended to be a base class for object oriented specialisations of Term. There is no practical advantage of the type safety given by this change when used in the context of mapping.

Could you please clarify what you mean by wrapping out of context?

Baring the cost of moving away from easily comprehensible idiomatic typings for the sake of type safety that would practically never be used is not a good idea.

Type safety has three very practical implications:

  • It is easier to catch errors at build time, rather than run time. This is always a desirable feature. Both for library maintenance and consumption.
  • Developers have less conidence they are using a library correctly if they are over-using as. We are forcing developers into this position in cases such as using WrappedTerms in #match at present.
  • There are some false types with the current design. Specifically, language is declared to be defined, even on non-literal terms such as NamedNodes. We want to ensure people do not inadvertently be using this property whe they should not be.

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.

4 participants