From 094b92debb60e7f61535fe19bd12553dac442806 Mon Sep 17 00:00:00 2001 From: Joep Meindertsma Date: Mon, 18 Nov 2019 15:56:56 +0100 Subject: [PATCH] Fether typings #355 --- src/collection.ts | 2 +- src/fetcher.ts | 241 ++++++++++++++++++++++++---------------------- src/formula.ts | 5 +- src/util.js | 2 +- src/wip.md | 13 ++- 5 files changed, 142 insertions(+), 121 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index c6d4ea089..5c3a15dbf 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -70,7 +70,7 @@ export default class Collection extends Node { * Gets a new Collection with the substituting bindings applied * @param bindings The bindings to substitute */ - //@ts-ignore Incompatible signature with Node.substitute + // @ts-ignore Incompatible signature with Node.substitute substitute(bindings: Bindings): Collection { var elementsCopy = this.elements.map(function (ea) { ea.substitute(bindings) diff --git a/src/fetcher.ts b/src/fetcher.ts index 570fd79e0..8b38c6785 100644 --- a/src/fetcher.ts +++ b/src/fetcher.ts @@ -41,7 +41,6 @@ import serialize from './serialize' import { fetch as solidAuthCli } from 'solid-auth-cli' import { fetch as solidAuthClient } from 'solid-auth-client' import { TFBlankNode, TFNamedNode, TFTerm, TFGraph, TFSubject, ContentType } from './types' -import { Formula } from './index' import Literal from './literal' // This is a special fetch which does OIDC auth, catching 401 errors @@ -86,21 +85,15 @@ const ns = { interface FetchError extends Error { statusText?: string status?: StatusValues - response?: ResponseType + response?: ExtendedResponse } -// Both Fetch like Response objects as InternalResponse types are allowed -type ResponseType = Response | InternalResponse | Partial - -/** Internal type for Responses. Resembles the Response type for .fetch() */ -interface InternalResponse { - /** Originally, this just allows numbers, but RDFLib also uses other status types */ - status?: StatusValues - /** The request meta blank node */ - req?: TFNamedNode - statusText?: string +/** An extended interface of Response, since RDFlib.js adds some properties. */ +interface ExtendedResponse extends Response { + /** String representation of the Body */ responseText?: string - url?: string + /** Identifier of the reqest */ + req?: TFSubject size?: number timeout?: number } @@ -189,12 +182,12 @@ interface AutoInitOptions extends RequestInit{ class Handler { // TODO: Document, type - response: ResponseType + response: ExtendedResponse // TODO: Document, type dom: Document static pattern: RegExp - constructor (response: ResponseType, dom?: Document) { + constructor (response: ExtendedResponse, dom?: Document) { this.response = response // The bang operator here might need to be removed. this.dom = dom! @@ -219,6 +212,7 @@ class RDFXMLHandler extends Handler { /** Requires .original */ options: { original: TFSubject + req: TFSubject } & Options, ) { let kb = fetcher.store @@ -263,7 +257,7 @@ class XHTMLHandler extends Handler { resource: TFSubject original: TFSubject } & Options, - ): Promise | ResponseType { + ): Promise | ExtendedResponse { let relation, reverse: boolean if (!this.dom) { this.dom = Util.parseXML(responseText) @@ -298,9 +292,9 @@ class XHTMLHandler extends Handler { for (let i = 0; i < scripts.length; i++) { let contentType = scripts[i].getAttribute('type') if (Parsable[contentType!]) { - //@ts-ignore incompatibility between Store.add and Formula.add + // @ts-ignore incompatibility between Store.add and Formula.add rdfParse(scripts[i].textContent as string, kb, options.original.value, contentType) - //@ts-ignore incompatibility between Store.add and Formula.add + // @ts-ignore incompatibility between Store.add and Formula.add rdfParse(scripts[i].textContent as string, kb, options.original.value, contentType) } } @@ -315,11 +309,11 @@ class XHTMLHandler extends Handler { } catch (err) { let msg = 'Error trying to parse ' + options.resource + ' as RDFa:\n' + err + ':\n' + err.stack - return fetcher.failFetch(options, msg, 'parse_error') + return fetcher.failFetch(options as AutoInitOptions, msg, 'parse_error') } } - return fetcher.doneFetch(options, this.response) + return fetcher.doneFetch(options as AutoInitOptions, this.response) } } XHTMLHandler.pattern = new RegExp('application/xhtml') @@ -342,7 +336,7 @@ class XMLHandler extends Handler { req: TFBlankNode resource: TFSubject } & Options, - ): ResponseType | Promise { + ): ExtendedResponse | Promise { let dom = Util.parseXML(responseText) // XML Semantics defined by root element namespace @@ -421,11 +415,11 @@ class HTMLHandler extends Handler { fetcher: Fetcher, responseText: string, options: { - req: TFSubject, + req: TFBlankNode, resource: TFSubject, original: TFSubject, } & Options - ): Promise | ResponseType { + ): Promise | ExtendedResponse { let kb = fetcher.store // We only handle XHTML so we have to figure out if this is XML @@ -490,7 +484,7 @@ class TextHandler extends Handler { original: TFSubject resource: TFSubject } & Options - ): ResponseType | Promise { + ): ExtendedResponse | Promise { // We only speak dialects of XML right now. Is this XML? // Look for an XML declaration @@ -545,10 +539,9 @@ class N3Handler extends Handler { options: { original: TFNamedNode req: TFSubject - } - & Options, - response: ResponseType - ): ResponseType | Promise { + } & Options, + response: ExtendedResponse + ): ExtendedResponse | Promise { // Parse the text of this N3 file let kb = fetcher.store let p = N3Parser(kb, kb, options.original.value, options.original.value, @@ -608,6 +601,8 @@ type StatusValues = 404 | /** In attempt to counter CORS problems retried */ 'redirected' | + /** If it did fail */ + 'failed' | 'parse_error' | /** * URI is not a protocol Fetcher can deal with @@ -633,27 +628,27 @@ interface TimeOutsMap { [uri: string]: NodeJS.Timeout[] } -interface RedirectedToMap { - [uri: string]: string -} - interface FetchQueue { - [uri: string]: Promise + [uri: string]: Promise } interface FetchCallbacks { - [uri: string]: Function[] + [uri: string]: UserCallback[] } interface BooleanMap { [uri: string]: boolean } -// Not sure about the shapes of this. Response? Strin? -type Result = ResponseType +// Not sure about the shapes of this. Response? FetchError? +type Result = Response /** Differs from normal Fetch, has an extended Response type */ -type Fetch = (input: RequestInfo, init?: RequestInit) => Promise; +type Fetch = (input: RequestInfo, init?: RequestInit) => Promise; + +interface CallbackifyInterface { + fireCallbacks: Function +} /** Fetcher * @@ -663,7 +658,7 @@ type Fetch = (input: RequestInfo, init?: RequestInit) => Promise; * figuring how to parse them. It will also refresh, remove, the data * and put back the fata to the web. */ -export default class Fetcher { +export default class Fetcher implements CallbackifyInterface { store: IndexedFormula timeout: number _fetch: Fetch @@ -686,8 +681,8 @@ export default class Fetcher { requested: RequestedMap /** List of timeouts associated with a requested URL */ timeouts: TimeOutsMap - /** When 'redirected' */ - redirectedTo: RedirectedToMap + /** Redirected from *key uri* to *value uri* */ + redirectedTo: Record fetchQueue: FetchQueue /** fetchCallbacks[uri].push(callback) */ fetchCallbacks: FetchCallbacks @@ -698,9 +693,13 @@ export default class Fetcher { static HANDLERS: { [handlerName: number]: Handler } + static CONTENT_TYPE_BY_EXT: Record // TODO: Document this static crossSiteProxyTemplate: any + /** Methods added by calling Util.callbackify in the constructor*/ + fireCallbacks!: Function + constructor ( store: IndexedFormula, options: Options = {} @@ -906,7 +905,7 @@ export default class Fetcher { load ( uri: TFNamedNode | string | Array, options: Options = {} - ): Promise { + ): Promise | Result[] { options = Object.assign({}, options) // Take a copy as we add stuff to the options!! if (uri instanceof Array) { return Promise.all( @@ -925,7 +924,7 @@ export default class Fetcher { pendingFetchPromise ( uri: string, originalUri: string, - options: Options + options: AutoInitOptions ): Promise { let pendingPromise @@ -1016,8 +1015,8 @@ export default class Fetcher { */ fetchUri ( docuri: string, - options: Options - ): Promise | ResponseType { + options: AutoInitOptions + ): Promise { if (!docuri) { return Promise.reject(new Error('Cannot fetch an empty uri')) } @@ -1031,6 +1030,7 @@ export default class Fetcher { if (!options.force) { if (state === 'fetched') { // URI already fetched and added to store return Promise.resolve( + // @ts-ignore This is not a valid response object this.doneFetch(options, { status: 200, ok: true, @@ -1040,7 +1040,8 @@ export default class Fetcher { } if (state === 'failed' && this.requested[docuri] === 404) { // Remember nonexistence let message = 'Previously failed: ' + this.requested[docuri] - let dummyResponse: ResponseType = { + // @ts-ignore This is not a valid response object + let dummyResponse: ExtendedResponse = { url: docuri, // This does not comply to Fetch spec, it can be a string value in rdflib status: this.requested[docuri] as number, @@ -1071,12 +1072,12 @@ export default class Fetcher { let { actualProxyURI } = options - //@ts-ignore - return this._fetch(actualProxyURI, options) + return this._fetch((actualProxyURI as string), options) .then(response => this.handleResponse(response, docuri, options), error => { // @@ handleError? - let dummyResponse = { - url: actualProxyURI, + // @ts-ignore Invalid response object + let dummyResponse: ExtendedResponse = { + url: actualProxyURI as string, status: 999, // @@ what number/string should fetch failures report? statusText: (error.name || 'network failure') + ': ' + (error.errno || error.code || error.type), @@ -1148,8 +1149,8 @@ export default class Fetcher { options = p2 } - this.load(uri, options) - .then((fetchResponse: ResponseType) => { + (this.load(uri, options) as Promise) + .then((fetchResponse: ExtendedResponse) => { if (userCallback) { if (fetchResponse) { if ((fetchResponse as Response).ok) { @@ -1157,8 +1158,8 @@ export default class Fetcher { } else { // console.log('@@@ fetcher.js Should not take this path !!!!!!!!!!!!') let oops = 'HTTP error: Status ' + fetchResponse.status + ' (' + fetchResponse.statusText + ')' - if ((fetchResponse as InternalResponse).responseText) { - oops += ' ' + (fetchResponse as InternalResponse).responseText // not in 404, dns error, nock failure + if (fetchResponse.responseText) { + oops += ' ' + fetchResponse.responseText // not in 404, dns error, nock failure } console.log(oops + ' fetching ' + uri) userCallback(false, oops, fetchResponse) @@ -1214,12 +1215,12 @@ export default class Fetcher { */ failFetch ( options: { - req: TFSubject + req: TFBlankNode original: TFSubject } & Options, errorMessage: string, statusCode: StatusValues, - response?: ResponseType + response?: ExtendedResponse ): Promise { this.addStatus(options.req, errorMessage) @@ -1329,14 +1330,14 @@ export default class Fetcher { req: TFSubject, original: TFSubject } & Options, - response: ResponseType - ): ResponseType { + response: ExtendedResponse + ): Response { this.addStatus(options.req, 'Done.') this.requested[options.original.value] = 'done' this.fireCallbacks('done', [options.original.value]) - (response as InternalResponse).req = options.req // Set the request meta blank node + response.req = options.req // Set the request meta blank node return response } @@ -1365,7 +1366,7 @@ export default class Fetcher { putBack ( uri: TFNamedNode | string, options: Options = {} - ): Promise { + ): Promise { uri = (uri as TFNamedNode).value || uri // Accept object or string let doc = new NamedNode(uri).doc() // strip off # options.contentType = options.contentType || 'text/turtle' @@ -1373,7 +1374,7 @@ export default class Fetcher { return this.webOperation('PUT', uri, options) } - webCopy (here: string, there: string, contentType): Promise { + webCopy (here: string, there: string, contentType): Promise { return this.webOperation('GET', here) .then((result) => { return this.webOperation( @@ -1382,7 +1383,7 @@ export default class Fetcher { }) } - delete (uri: string, options: Options): Promise { + delete (uri: string, options: Options): Promise { return this.webOperation('DELETE', uri, options) .then(response => { this.requested[uri] = 404 @@ -1402,7 +1403,7 @@ export default class Fetcher { doc: NamedNode, contentType = 'text/turtle', data = '' - ): Promise { + ): Promise { const fetcher = this try { var response = await fetcher.load(doc) @@ -1424,7 +1425,7 @@ export default class Fetcher { } } // console.log('createIfNotExists: doc exists, all good: ' + doc) - return response + return response as Response } /** @@ -1447,6 +1448,7 @@ export default class Fetcher { headers['slug'] = folderName } + // @ts-ignore These headers lack some of the required operators. let options: Options = { headers } if (data) { @@ -1456,8 +1458,8 @@ export default class Fetcher { return this.webOperation('POST', parentURI, options) } - invalidateCache (uri) { - uri = uri.value || uri // Allow a NamedNode to be passed as it is very common + invalidateCache (uri: string | TFNamedNode): void { + uri = nodeValue(uri) const fetcher = this if (fetcher.fetchQueue && fetcher.fetchQueue[uri]) { console.log('Internal error - fetchQueue exists ' + uri) @@ -1490,7 +1492,7 @@ export default class Fetcher { uriIn: string | TFNamedNode, // Not sure about this type. Maybe this Options is different? options: Options = {} - ): Promise { + ): Promise { const uri = nodeValue(uriIn) options.method = method options.body = options.data || options.body @@ -1508,13 +1510,13 @@ export default class Fetcher { return new Promise(function (resolve, reject) { fetcher._fetch(uri, options).then(response => { - if ((response as Response).ok) { + if (response.ok) { if (method === 'PUT' || method === 'PATCH' || method === 'POST' || method === 'DELETE') { fetcher.invalidateCache (uri) } - if ((response as Response).body) { - (response as Response).text().then(data => { - (response as InternalResponse).responseText = data + if (response.body) { + response.text().then(data => { + response.responseText = data resolve(response) }) } else { @@ -1524,7 +1526,7 @@ export default class Fetcher { let msg = 'Web error: ' + response.status if (response.statusText) msg += ' (' + response.statusText + ')' msg += ' on ' + method + ' of <' + uri + '>' - if ((response as InternalResponse).responseText) msg += ': ' + (response as InternalResponse).responseText + if (response.responseText) msg += ': ' + response.responseText let e2: FetchError = new Error(msg) e2.response = response reject(e2) @@ -1545,7 +1547,10 @@ export default class Fetcher { * @param rterm - the resource which referred to this * (for tracking bad links) */ - lookUpThing (term: TFNamedNode, rterm: TFNamedNode): Promise { + lookUpThing ( + term: TFNamedNode, + rterm: TFNamedNode + ): Promise | Response[] { let uris = this.store.uris(term) // Get all URIs uris = uris.map(u => Uri.docpart(u)) // Drop hash fragments @@ -1591,12 +1596,10 @@ export default class Fetcher { return undefined } - /** - * - * @param docuri - * @param options - */ - saveRequestMetadata (docuri: string, options: Options) { + saveRequestMetadata ( + docuri: string, + options: AutoInitOptions + ) { let req = options.req let kb = this.store let rterm = options.referringTerm @@ -1731,15 +1734,15 @@ export default class Fetcher { delete this.requested[term.value] // So it can be loaded again } - addHandler (handler: RDFXMLHandler) { + addHandler (handler: Handler) { this.handlers.push(handler) - handler.register(this) + (handler as N3Handler).register(this) } retryNoCredentials ( docuri: string, options - ): void { + ): Promise { console.log('Fetcher: CORS: RETRYING with NO CREDENTIALS for ' + options.resource) options.retriedWithNoCredentials = true // protect against being called twice @@ -1752,7 +1755,7 @@ export default class Fetcher { this.addStatus(options.req, 'Abort: Will retry with credentials SUPPRESSED to see if that helps') - return this.load(docuri, newOptions) + return this.load(docuri, newOptions) as Promise } /** @@ -1767,7 +1770,7 @@ export default class Fetcher { const hostpart = Uri.hostpart const here = '' + document.location - return hostpart(here) && hostpart(uri) && hostpart(here) !== hostpart(uri) + return (hostpart(here) && hostpart(uri) && hostpart(here)) !== hostpart(uri) } /** @@ -1775,10 +1778,10 @@ export default class Fetcher { * with status of 0. */ handleError ( - response: ResponseType | Error, + response: ExtendedResponse | Error, docuri: string, - options: Options - ): Promise { + options: AutoInitOptions + ): Promise { if (this.isCrossSite(docuri)) { // Make sure we haven't retried already if (options.credentials && options.credentials === 'include' && !options.retriedWithNoCredentials) { @@ -1806,20 +1809,20 @@ export default class Fetcher { } // This is either not a CORS error, or retries have been made - return this.failFetch(options, message, (response as Response).status || 998, response) + return this.failFetch(options, message, (response as Response).status || 998, (response as Response)) } // deduce some things from the HTTP transaction addType ( rdfType: TFNamedNode, req: TFSubject, - kb: Formula, + kb: IndexedFormula, locURI: string ): void { // add type to all redirected resources too let prev = req if (locURI) { var reqURI = kb.any(prev, ns.link('requestedURI')) - if (reqURI && reqURI !== locURI) { + if (reqURI && reqURI.value !== locURI) { kb.add(kb.sym(locURI), ns.rdf('type'), rdfType, this.appNode) } } @@ -1834,8 +1837,8 @@ export default class Fetcher { if (!response) { break } var redirection = kb.any((response as TFNamedNode), kb.sym('http://www.w3.org/2007/ont/http#status')) if (!redirection) { break } - //@ts-ignore always true? - if (redirection !== '301' && redirection !== '302') { break } + // @ts-ignore always true? + if ((redirection !== '301') && (redirection !== '302')) { break } } } @@ -1843,10 +1846,10 @@ export default class Fetcher { * Handle fetch() response */ handleResponse ( - response: ResponseType, + response: ExtendedResponse, docuri: string, - options: Options - ): Promise { + options: AutoInitOptions + ): Promise | ExtendedResponse { const kb = this.store const headers: Headers = (response as Response).headers @@ -1869,7 +1872,7 @@ export default class Fetcher { if (response.status >= 400) { if (response.status === 404) { - this.nonexistent[options.original.uri] = true + this.nonexistent[options.original.value] = true this.nonexistent[docuri] = true } @@ -1881,8 +1884,8 @@ export default class Fetcher { }) } - var diffLocation = null - var absContentLocation = null + var diffLocation: null | string = null + var absContentLocation: null | string = null if (contentLocation) { absContentLocation = Uri.join(contentLocation, docuri) if (absContentLocation !== docuri) { @@ -1916,7 +1919,7 @@ export default class Fetcher { // If we have already got the thing at this location, abort if (contentLocation) { - if (!options.force && diffLocation && this.requested[absContentLocation] === 'done') { + if (!options.force && diffLocation && this.requested[absContentLocation as string] === 'done') { // we have already fetched this // should we smush too? // log.info("HTTP headers indicate we have already" + " retrieved " + @@ -1924,12 +1927,12 @@ export default class Fetcher { return this.doneFetch(options, response) } - this.requested[absContentLocation] = true + this.requested[absContentLocation as string] = true } - this.parseLinkHeader(headers.get('link'), options.original, reqNode) + this.parseLinkHeader(headers.get('link') as string, options.original, reqNode) - let handler = this.handlerForContentType(contentType, response) + let handler = this.handlerForContentType(contentType, response) as Handler if (!handler) { // Not a problem, we just don't extract data @@ -1940,11 +1943,14 @@ export default class Fetcher { return response.text() .then(responseText => { response.responseText = responseText - return handler.parse(this, responseText, options, response) + return (handler as N3Handler).parse(this, responseText, options, response) }) } - saveErrorResponse (response: ResponseType, responseNode: TFNamedNode) { + saveErrorResponse ( + response: ExtendedResponse, + responseNode: TFSubject + ): Promise { let kb = this.store return response.text() @@ -1955,7 +1961,7 @@ export default class Fetcher { }) } - handlerForContentType (contentType: string, response: ResponseType): Handler | null { + handlerForContentType (contentType: string, response: ExtendedResponse): Handler | null { if (!contentType) { return null } @@ -1971,7 +1977,10 @@ export default class Fetcher { return CONTENT_TYPE_BY_EXT[uri.split('.').pop() as string] } - normalizedContentType (options: Options, headers: Headers): ContentType | string | null { + normalizedContentType ( + options: AutoInitOptions, + headers: Headers + ): ContentType | string | null { if (options.forceContentType) { return options.forceContentType } @@ -1985,7 +1994,7 @@ export default class Fetcher { } } - let protocol = Uri.protocol(options.resource.value) + let protocol = Uri.protocol(options.resource.value) as string if (!contentType && ['file', 'chrome'].includes(protocol)) { return 'text/xml' @@ -1999,11 +2008,8 @@ export default class Fetcher { */ redirectToProxy ( newURI: string, - options: { - req: TFSubject, - resource: TFSubject, - } & Options - ): Promise { + options: AutoInitOptions + ): Promise { this.addStatus(options.req, 'BLOCKED -> Cross-site Proxy to <' + newURI + '>') options.proxyUsed = true @@ -2032,7 +2038,13 @@ export default class Fetcher { }) } - setRequestTimeout (uri: string, options: Options): Promise { + setRequestTimeout ( + uri: string, + options: { + req: TFSubject + original: TFSubject + } & Options + ): Promise { return new Promise((resolve) => { this.timeouts[uri] = (this.timeouts[uri] || []).concat(setTimeout(() => { if (this.isPending(uri) && @@ -2044,7 +2056,10 @@ export default class Fetcher { }) } - addFetchCallback (uri, callback) { + addFetchCallback ( + uri: string, + callback: UserCallback + ): void { if (!this.fetchCallbacks[uri]) { this.fetchCallbacks[uri] = [callback] } else { diff --git a/src/formula.ts b/src/formula.ts index f467bf327..ec8586d78 100644 --- a/src/formula.ts +++ b/src/formula.ts @@ -728,7 +728,7 @@ export default class Formula extends Node { */ list(values: [], context: IndexedFormula): Collection | TFBlankNode { if (context.rdfFactory.supports["COLLECTIONS"]) { - //@ts-ignore if a rdfFactory supports collections, the collection() method should work + // @ts-ignore if a rdfFactory supports collections, the collection() method should work const collection = context.rdfFactory.collection() values.forEach(function (val) { collection.append(val) @@ -786,6 +786,7 @@ export default class Formula extends Node { var sts: TFQuad[] var sz sz = Serializer(this) + // @ts-ignore Formula.namespaces does not exist sz.suggestNamespaces(this.namespaces) sz.setBase(base) if (provenance) { @@ -820,7 +821,7 @@ export default class Formula extends Node { }) console.log('Formula subs statmnts:' + statementsCopy) var y = new Formula() - // This will throw an error, since this.add() can't handle + // @ts-ignore This will throw an error, since Formula.add() can't handle arrays y.add(statementsCopy) console.log('indexed-form subs formula:' + y) return y diff --git a/src/util.js b/src/util.js index f62ae527b..01ec4c47d 100644 --- a/src/util.js +++ b/src/util.js @@ -25,7 +25,7 @@ export function linkRelationProperty(relation){ * They return true if they want to be called again. * @method callbackify * @param obj {Object} - * @param callbacks {Array} + * @param callbacks {Array} */ export function callbackify (obj, callbacks) { obj.callbacks = {} diff --git a/src/wip.md b/src/wip.md index df7b22ec7..50c1c0fa4 100644 --- a/src/wip.md +++ b/src/wip.md @@ -40,23 +40,25 @@ Builds upon the approved #363 PR for [typescript migration](https://github.com/l - Converted `kb.add(someString)` to `kb.add(new Namednode(somestring))` to enhance compatibility with other datafactories. This happens in `Fetcher` and - `Fetcher.refreshIfExpired` passed an array of headers, but it needs only one string. - `Fethcer` uses `Headers` a lot. I've changed empty objects to empty `new Headers` instances, which enhances compatibility with default `Fetch` behavior. +- `Serializer.tripleCallback` had an unused third argument. ## Possible bugs not fixed by this PR -- The `Parse.executeErrorCallback` conditional logic is always `true`. - `Formula.substitute` uses `this.add(Statments[])`, which will crash. I think it should be removed, since `IndexedFormula.substitute` is used all the time anyway. - The `Formula.serialize` function calls `serialize.ts` with only one argument, so without a store. I think this will crash every time. Also`Formula.serialize` uses `this.namespaces`, but this is only defined in `IndexedFormula`. Is it rotten code and should it be removed? - `store.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, create `NamedNodes` from `URL` objects and `strings` that look like URLs . In any case, I thinkg the `Node.fromValue` behavior is too unpredictable for `store.add`. For now, I've updated the docs to match its behavior. - The types for `Node.fromValue` and `Literal.fromValue` show how unpredictable these methods are. I suggest we make them more strict (also relates to #362), so they either return a `TFTerm` (`node`) or throw an error - they should not return `undefined` or `null`. Also, I think they should be converted to functions in `Utils`: this would fix the circular dependency issue (why we need `node_internal`) and it would fix the type issues in `Literal.fromValue` (which tends to give issues since it's signature does not correctly extend from `Node.fromValue`) - In `Fetcher.addtype`, the final logic will allways return `true`, since `redirection` is a `NamedNode`. Should it call `.value`? - The `defaultGraph` iri is set to `chrome:theSession`, but this errors in Firefox. I suggest we change it to something else. See #370. +- The `Parse.executeErrorCallback` conditional logic is always `true`. +- Many `// @ts-ignore` comments for possible bugs. ## Unused code - `IndexedFormula.predicateCallback` is checked, but never used in this codebase. - The `optional` argument in `formula.js` does not seem to be documented, used or tested - should it be removed? -## Other things I noticed: +## Other things I noticed - Literals can apparently be `null` or `undefined`, when nodes are created using the `.fromValue` method. This causes faulty behavior. This happens in the `new Statement()` constructor as well. See #362. - The `IndexedFormula.add()` method has logic for Statement array inputs and store inputs, but this behavior is not documented. It also refers to `this.fetcher` and `this.defaultGraph`, which both should not be available. @@ -64,10 +66,14 @@ Builds upon the approved #363 PR for [typescript migration](https://github.com/l - Aliases (e.g. `IndexedFormula.match` for `IndexefFormula.statementsMatching`) introduce complexity, documentation and type duplication. I suggest adding deprecation warnings. - The various calling methods of `Fetcher.nowOrWhenFetched` are quite dynamic. A simpler, stricter input type might be preferable. - The Variable type (or `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 extending `Response` types, since these only use numbers. I feel we should only use the `499` browser error and add the text message to the `requestbody`. I've created a type for the internal `InternalResponse`; it shows how it differs from a regular `Response`. +- `Fetcher` `StatusValues` can be many types in RDFlib: string, number, true, undefined... This breaks compatibility with extending `Response` types, since these only use numbers. I feel we should only use the `499` browser error and add the text message to the `requestbody`. I've created a type for the internal `InternalResponse`; it shows how it differs from a regular `Response`. The `.responseText` property, for example, is used frequently in Fetcher, but - The `IndexedFormula` and `Formula` methods have incompatible types, such as in `compareTerm`, `variable` and `add`. I've added `//@ts-ignore` lines with comments. +- The fourth `reponse` argument in `.parse()` methods in `Handler` classes was unused (except in N3Handler), so I removed it everywhere it was unused. - `Serializer`'s fourth `options` argument is undocumented, and I couldn't find out how it worked. - `Fetcher` `saveResponseMetadata` creates literals +- Many functions in `Fetcher` assume that specific `Opts` are defined. I've included all these in a single `Options` type and added documentation for the props I understood. I've also created an `AutoInitOptions` 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 be `Opts`, 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[]`. I suggest splitting this in two functions, e.g. add `loadMany` +- `Utils.callbackify` seems to be used only in `Fetcher`. ## Some thoughts on simplifying language @@ -83,4 +89,3 @@ Therefore, we should try to use consistent langauge and keep synonyms to a minim ## Probably OK, but I don't get it: - I'm a bit embarassed about this, but even after rewriting so much of the code I still don't understand all methods. E.g. `Forumla.transitiveClosure()` -- Some functions in `Fetcher` assume that specific `Opts` are defined. I've included all these in a single `Options` type (and an `AutoInitOptions` type which sets auto-initialized opts), and extended this type in each function where specific opts seemed to be required. I'm not entirely confident about the types I've set.