Issue #45: Add missing spec methods to DataFactory interface

[matthieubosquet]

As per issue #45 the fromTerm and fromQuad DataFactory methods defined by the RDF/JS DataModel specification are not reflected in the @rdfjs/types package.

This PR adds those methods.

That said, I am unsure in which context those methods would be used and didn't find an explanation of when it would be or whether the omission is intentional.

[changeset-bot]

🦋 Changeset detected

Latest commit: cfc4d28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@rdfjs/types	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jeswr]

Hi @matthieubosquet :)

Thanks for these changes! It may be worth using a mver bump to release these changes, as many RDF/JS implementations do not implement these methods (c.f. rdfjs/N3.js#447) - and so this will either cause (1) the package to declare methods that it does not implement or (2) have type errors if the package is, e.g., written in typescript and implements the DataFactory interface.

[matthieubosquet]

Rebased and made this a major bump: b0ca1a9.

I am not sure what is the release process and how to publish a v1.1.1 (maybe before merging this).

I also wonder if maybe we could add a few items before potentially publishing a version 2.0.0:

1. simplify the release process, document it and update the publish action accordingly (it feels like changeset and yarn might be a tad overkill and extra tooling to learn)
2. update dependencies (typescript at least, hopefully eslint too)

[tpluscode]

Please see inline comment

[tpluscode]

If we want to simultaneously work on v2 then we'd probably need a dedicated branch...

[tpluscode]

Thanks for the changeset @matthieubosquet 👌

I took a closer look and I agree with the other to make this a major release. A little unfortunate, because this may disrupt the ecosystem when some packages start to depend on @rdfjs/types@^2 while other stay on v1. But maybe I'm wrong we'll see

I also added some more suggestions (was on mobile earlier)

[matthieubosquet]

Thanks for the suggestions @tpluscode and @rubensworks!

I fixed up the tests after applying suggestions: f1879fb.

The DataFactory initialised with const dataFactory: DataFactory<BaseQuad> = <any> {}; was returning a BaseQuad, not a Quad, hence CI failure.

[tpluscode]

Great work. Thanks. I think we're good now

[rubensworks]

Tested in rdf-data-factory, and everything seems to work as intended!

[jeswr]

BaseQuad input should also presumably be supported since BaseQuads are valid Terms

[matthieubosquet]

After @jeswr's comment, I realised that there might be a bit of inconsistency in the DataFactory method declarations and its use of generic types.

In a nutshell, with a typing such as: fromTerm<T extends NamedNode>(original: T): NamedNode;, the DataFactory would take in an extended NamedNode and produce a NamedNode.
I don't think that's what we want for DataFactory implementers.
With a typing such as: fromTerm<T extends Term>(original: T): T;, if we pass an extended NamedNode to the DataFactory, it will return the extended type which still can be cast to a NamedNode (see corresponding test).
Isn't it closer to what DataFactory implementers might want?

---

I tried to produce a commit that illustrates and offers a compromise to fix the issue.

---

Remains an inconsistency with the quad factory part:
We are currently offering to define InQuad and OutQuad as anything that extends BaseQuad.
That allows us to be specific as to what the DataFactory consumes and produces (as far as quads are concerned).

If we allow that amount of specificity for Quads, is there a specific reason not to allow it for other Terms?

[jeswr]

I don't think that's what we want for DataFactory implementers.

I think I see where you're going, but this solution doesn't quite work. Consider using fromTerm from @rdfjs/data-model with an input of an N3.js NamedNode. The N3.js NamedNode has an extra #toJson method that will not be available in the NamedNode output by the @rdfjs/data-model method; however the type inference you have now would suggest that the output NamedNode does have this #toJson method.

What you want to allow for is for the libraries to extend the DataFactory types such that any custom extensions they have to their terms can be included - that is fromTerm from N3.js will always output a NamedNode with the #toJson method regardless of whether the input NamedNode does or not. Your original implementation should permit this.

[tpluscode]

To demonstrate the problem, here's see this playground link with the generic function.

Here's what @jeswr described. The var converted is typed as NamedNodeExt which is probably incorrect.

[tpluscode]

I think it's worth mentioning the PR #24 which could allow implementors to define concrete term subtypes returned by a data factory

@blake-regalia would you interested in picking up where we left that?

[jeswr]

@tpluscode and any interested others in this thread; could you please review DefinitelyTyped/DefinitelyTyped#70614

[tpluscode]

Can we put this back to the state as it was approved earlier? @matthieubosquet

[tpluscode]

Thanks @matthieubosquet!

Unfortunately, that is not what I had in mind. I propose that you revert this branch to the commit f1879fb + apply @jeswr's comment

I think that means

-fromTerm<T extends Quad>(original: T): OutQuad
+fromTerm<T extends BaseQuad>(original: T): OutQuad

[matthieubosquet]

@tpluscode does that look alright to you?

[tpluscode]

It's been a long time but I believe we're good here now

[jeswr]

LGTM