Closed joepio closed 4 years ago
I tried (see commit) replacing my TF types with those from @types/rdf-js, but it seems these are incompatible - both with RDFlib and with the original spec.
One of the most fundamental problems is that @types/rdf-js defines Term
as a Union of its other Terms:
export type Term = NamedNode | BlankNode | Literal | Variable | DefaultGraph;
The RDF/JS spec defines Term as an abstract interface.
Since RDFlib uses custom terms (like Collection), it is compatible with the RDF/JS spec, and it works with my types, but it does not work with the types from @types/rdf-js. This problem with Term, unfortunately, trickles down quite a bit and makes it impossible to switch, at the moment.
There are a couple of things we can do here:
@RubenVerborgh What are your thoughts on this?
To me it sounds like we want to start a process to make sure that the interfaces expressed by RDFJS are compatible with the work in rdflib. That is a lengthier discussion though, so I would suggest removing those parts and keep this PR simple.
We could of course mitigate that for now by having src/tf-types
, but I think removing them altogether is better as a first step?
To me it sounds like we want to start a process to make sure that the interfaces expressed by RDFJS are compatible with the work in rdflib. That is a lengthier discussion though, so I would suggest removing those parts and keep this PR simple.
I'll make a separate issue for migrating to external types. (Edit: #374)
We could of course mitigate that for now by having
src/tf-types
, but I think removing them altogether is better as a first step?
Removing them has quite a big impact, I'd suggest we use the tf-types
for now and do the migration to an external type definition later. As for now, the types in this PR seem to resemble the spec more closely.
@megoth
I believe testing in other applications (e.g. in mashlib, some node env) is smart before doing a definitive release and bumping the version. U agree that a pre-release is the way to go!
About src/tf-types.ts
, why do we copy instead of use? I only see disadvantages. There are more stakeholders to the RDF/JS domain than the maintainer of that one package. If there are incompatibilities, the package should be adjusted.
Also, we should use proper naming. The word "taskforce" shouldn't occur in code, comments, or anywhere else. The proper terminology is the RDF/JS Data Model specification: https://rdf.js.org/data-model-spec/
@RubenVerborgh We copied from the @types/rdflib
since these were the types specific to this repo. Since the repo is migrated to typescript in this PR, the @types/rdflib
types are no longer useful - the ones in this PR are more accurate anyway, since they have to be fully internally consistent to even compile. For why I'm not using @types/rdfjs
, see #374 (in short: I'm working on a PR to change Term, hopefully we can use these types after that is merged).
I've removed the 'taskforce' mentions.
@joepio Thanks, lost track of all the issues. Only remnants now are TF
prefixes as in TFDataFactory
, which should also be replaced. Perhaps RdfJsDataFactory
or RSDataFactory
.
Happy to withdraw my objections after the last commits, and if we are using #374 to ensure that the interfaces are in fact copies and thus straight imports. If we can't something is wrong on either our side or their side.
This PR converts most of RDFlib.js to Typescript (#355). It fixes various minor bugs and it adds a lot of documentation, using TypeDoc instead of JSDoc. Since it's such a big PR with most of the code changed, I've added a changelog with some of my thoughts. For the most important ones, I've already opened some issues, but for the less important ones I've just added some bullets below.
Changes included in PR #363 (already approved)
Node
,Literal
,BlankNode
,NamedNode
,Collection
,Statement
..types
file for shared types.@types/rdflib
, this makes the documentation far more complete.@types/rdflib
by Cénotélie. Added credits inpackage.json
, discussed this with Cénotélie.New changes
formula
,variable
,store
,update-manager
,data-factory
,default-graph
,namespace
,parse
,serialize
,parse
,uri
andutils
tot ts.fork-ts-checker-webpack-plugin
, which enables errors in the log when ts errors occur. Since this gave errors, we usetsc
in theprepare
script. This means that only builds without type errors will pass.isTFNamedNode
andisTFPredicate
inUtils
, and used these at various locations.termType
andcontentType
, without breaking compatibility with regular strings.Formula
Constructor arguments are optional - since some functions initialize empty Formulas.Formula.fromNT()
return this.literal(str, lang || dt)
seemed wrong, converted it tofromValue
methods conflict with the base Node class, type wise. Since they don't usethis
, I feel like they should be converted to functions.Compatibility with RDFJS taskforce and external datafactories
We've worked quite a bit on compliance with the TF (TaskForce) spec. The internal types (including the factory) extend Taskforce types, which live in a separate
tf-types.ts
file. This makes it easier to use RDFLib in combination with external, custom data factories.sameTerm
toequals
in order to comply to TF spec, so that these functions also work with external datafactories. Alias still exists, so nothing changes externally..why
to.graph
. The.why
alias still exists, so nothing changes for external users.kb.sym
have been replaced withkb.rdfFactory.namedNode
, which makes all these functions more compatible with external datafactories. Added a deprecation warning to.sym
.Minor fixes
Formula.holds()
, since it did not make senseNode.substitute()
.justOne
argument fromformula.statementsMatching
, since it was unused.uri.document
function called.uri
on a string, I removed that.parser.parse
function infetcher.parse
, since the function only takes three.response
argument fromfetcher.parse
,XHTMLHandler.parse
,RDFXMLHander.parse
,XMLHandler
, since it was not used.Fetcher.failfetch
added strings as objects to the store. Changed that to literals.NamedNode.uri
are changed to.value
to comply with TF spec. This enables these functions to work with external datafactories.Fetcher.cleanupFetchRequest
Options
type for Fetcher. Not sure if this is the way to go.Node.toJS
, the boolean only returned true if thexsd:boolean
value is'1'
, now it it should also work for'true'
.kb.add(someStrin2g)
tokb.add(new Namednode(somestring))
to enhance compatibility with other datafactories. This happens inFetcher
andFetcher.refreshIfExpired
passed an array of headers, but it needs only one string.Fethcer
usesHeaders
a lot. I've changed empty objects to emptynew Headers
instances, which enhances compatibility with defaultFetch
behavior.Serializer.tripleCallback
had an unused third argument.UpdateManager.update
checked an undefinedsecondTry
variable. Since later in the same function,.update
is called with a 4th argument, I assume this issecondTry
. I've added it as an optional argument. Perhaps this isFormula.add()
now usesthis.rdfFactory.defaultGraph
instead of the non-existentthis.defaultGraph
IndexedFormula.replaceWith
now passes a Node instead of a string to.add
in theif (big.value)
blocktrue
values inxsd:boolean
literals, according to xsd spec.Possible bugs discovered, which are not fixed by this PR
Formula.substitute
usesthis.add(Statments[])
, which will crash. I think it should be removed, sinceIndexedFormula.substitute
is used all the time anyway.Formula.serialize
function callsserialize.ts
with only one argument, so without a store. I think this will crash every time. AlsoFormula.serialize
usesthis.namespaces
, but this is only defined inIndexedFormula
. Is it rotten code and should it be removed?IndexedFormula.add()
accepts many types of inputs, but this will lead to invalid statements (e.g. a Literal as a Subject). I suggest we make this more strict and throw more errors on wrong inputs. Relates to #362. We could still make the allowed inputs bigger by allowing other types with some explicit behavior, e.g. in subject arguments, createNamedNodes
fromURL
objects andstrings
that look like URLs . In any case, I thinkg theNode.fromValue
behavior is too unpredictable forstore.add
. For now, I've updated the docs to match its behavior.Node.fromValue
andLiteral.fromValue
show how unpredictable these methods are. I suggest we make them more strict (also relates to #362), so they either return aTFTerm
(node
) or throw an error - they should not returnundefined
ornull
. Also, I think they should be converted to functions inUtils
: this would fix the circular dependency issue (why we neednode_internal
) and it would fix the type issues inLiteral.fromValue
(which tends to give issues since it's signature does not correctly extend fromNode.fromValue
)Fetcher.addtype
, the final logic will allways returntrue
, sinceredirection
is aNamedNode
. Should it call.value
?Hanlder.parse()
functions inFetcher
return either aResponse
or aPromise<Error>
. This seems like weird behavior - should it not always return an array?defaultGraph
iri is set tochrome:theSession
, but this errors in Firefox. I suggest we change it to something else. See #370.Parse.executeErrorCallback
conditional logic is alwaystrue
.// @ts-ignore
comments. Ideally, these should be resolved by fixing the underlying type issues.UpdateManager.update_statement
seems to refer to the wrongthis
. It callsthis.anonimize
, but it is not available in that scope.UpdateManager.updateLocalFile
usesComponent
, but this is not defined anywhere. Is this deprecated?Data-factory-internal.id()
returnsstring | undefined
, I feel like undefined should not be possible - it should throw an error. This would resolve the type incompatibility on line 146.IndexedFormula.copy
runs.copy
on a Collection, but that method is not available there.IndexedFormula.predicateCallback
is checked, but never used in this codebase.Other things I noticed
null
orundefined
, when nodes are created using the.fromValue
method. This causes faulty behavior. This happens in thenew Statement()
constructor as well. See #362.IndexedFormula.add()
method has logic for Statement array inputs and store inputs, but this behavior is not documented. It also refers tothis.fetcher
andthis.defaultGraph
, which both should not be available. I've added types that accept these arrays.store.ts
is calledIndexedFormula
.IndexedFormula.match
forIndexefFormula.statementsMatching
) introduce complexity, documentation and type duplication. I suggest adding deprecation warnings.Fetcher.nowOrWhenFetched
are quite dynamic. A simpler, stricter input type might be preferable.TFVariable
) really messes with some assumptions. I feel like they should not be valid in regular quads, since it's impossible to serialize them decently. If I'd add it to the accepted types, we'd require a lot of typeguards in functions.Fetcher
StatusValues
can be many types in RDFlib: string, number, true, undefined... This breaks compatibility with extendingResponse
types, since these only use numbers. I feel we should only use the499
browser error and add the text message to therequestbody
. I've created a type for the internalInternalResponse
; it shows how it differs from a regularResponse
. The.responseText
property, for example, is used frequently in Fetcher, butIndexedFormula
andFormula
methods have incompatible types, such as incompareTerm
,variable
andadd
. I've added//@ts-ignore
lines with comments.reponse
argument in.parse()
methods inHandler
classes was unused (except in N3Handler), so I removed it everywhere it was unused.Serializer
's fourthoptions
argument is undocumented, and I couldn't find out how it worked.Fetcher
saveResponseMetadata
creates literalsFetcher
assume that specificOpts
are defined. I've included all these in a singleOptions
type and added documentation for the props I understood. I've also created anAutoInitOptions
type, which sets auto-initialized. I extended Options in each function where specific opts seemed to be required. I'm not entirely confident about the types I've set. I feel like the truly required items should never beOpts
, since they aren't optional. Refactoring this requires a seperate PR.Fetcher.load
allows arrays as inputs. This causes the output types to be more unpredictable.Promise<Result> | Result[]
. I suggest splitting this in two functions, e.g. addloadMany
Utils.callbackify
seems to be used only inFetcher
.UpdateManager.editable
has a confusing return type (string | boolean | undefined
). I suggest we refactor it to always return one of multiple pre-defined strings,.optional
argument informula.js
does not seem to be documented, used or tested - should it be removed?Need review
Formula
andIndexedFormula
functions (e.g.anyStatementMatching
) might have too strict types - perhaps Collections are allowed in some of them.IndexedFormula.declareExistential
&newExistential
have different assumptions on types - should they be blanknodes or namednodes?IndexedFormula.check
passes a single statement tocheckStatementList
, which expects an arrayas TFObject
), but Ideally, these do not exist. Ultimately, these should be replaced by TypeGuards that work on runtimeSome thoughts on simplifying language
Getting started with Linked Data or RDF can be difficult, since it introduces quite a few new concepts. Since this library is powerful and generic, it might be one of the first and most important RDF tools that a developer will use. Therefore, we should try to use consistent langauge and keep synonyms to a minimum.
Node
andTerm
seem to refer to the same concept. Both are used in this repo. I think Term is slightly more suited, partially because it complies to the TF spec, but also because it seems more sementically correct. ALiteral
, for example, is not really a node in the mathematical sense, it's more of an edge, since it cannot connect to other nodes.Statement
,Triple
andQuad
refer to the same concept, at least technically. Maybe we could pick one. I suggestStatement
, because it covers both triples and quads.graph
is referred to aswhy
,doc
andgraph
in the code and API. I think this might be confusing - should we just call itgraph
everywhere?IndexedFormula
default export name is different from thestore
filename. It might be easier to just call itstore
everywhere, including where it's calledkb
.Notes for PR reviewer