Node.fromValue() type assumptions - null / undefined

[joepio]

The Node.fromValue method (which is called in the Statement constructor) creates an RDF Node (Term) from a JS value. If a new Statement is constructed with Null / undefined values, we'll have a statement with Subject / Predicate / Object values of null or undefined.

This behavior seems intentional, since it's documented:

 RDF Nodes are returned unchanged, undefined returned as itself.

This means that the types for subject, predicate and object should be:

  subject: Node | undefined | null;
  predicate: Node | undefined | null;
  object: Node | Collection | Literal | undefined | null;

Instead of:

  subject: Node;
  predicate: Node;
  object: Node | Collection | Literal;

These simpler types are easier to deal with, since it makes null / undefined checks unnecessary. Since a triple without a subject and / or predicate / object seems silly, I think it might be better to throw an error and keep the types strict.

The wrong value types in Statements can cause weird behavior:

const emptyStatement = new Statement(); // No errors when creating empty statement
console.log(emptyStatement.toString()); // Error: Cannot read property 'toNT' of undefined

const emptyStatement = new Statement(
  "http://example.com", // Becomes a literal
  "http://example.com", // Becomes a literal
  "http://example.com", // Becomes a literal
); // No errors when creating Statement with a literal as a subject
console.log(emptyStatement.toNT()); // No error, but it returns invalid N-Triples

I've used the stricter types in my fork for #355 in the Statement constructor. My suggestion is to throw an error when an invalid Statement is initialized. If we don't do this, we should deal with the consequences of having undefined / null as valid types, which means implementing behavior for existing methods (e.g. serialize).

I also think it might be better to move Node.fromValue and Literal.fromValue to util.ts. This fixes a couple of problems: the circular dependency fix (node-internal.ts) and the type incompatibility in the signatures for Literal.fromValue and Node.fromValue.

Related discussion in rdf/data-model-spec.

[joepio]

Just discussed this with @megoth and @timbl.

We agreed that new Statement(null, undefined, null) and other nonsensical statements should throw an error. This also means that NamedNode.fromValue() will throw if it is passed undefined or null, while currently it returns that value.

There was some discussion on how to deal with an undefined graph, though. Currently, it sets a defaultGraph(). We could throw an error if no explicit graph has been set, but this will could break many clients. Instead, we decided to log a warning to the console that this feature is at risk, linking to this issue.