N3.js icon indicating copy to clipboard operation
N3.js copied to clipboard

Outdated type information in @types/n3

Open renyuneyun opened this issue 2 years ago • 13 comments

As the title says, the type information in @types/n3 is outdated. There was only 1.10.4, while the current version for n3 is 1.16.1.

At least one incompatibility is the readQuads() method for Store. It exists in (at least) 1.16.0, while the type information in 1.10.4 does not have it.

Not sure if I should report this here or in https://github.com/DefinitelyTyped/DefinitelyTyped. But as the maintainers here also contributed to that repo, I guess starting from here is the best option.

renyuneyun avatar Apr 15 '22 18:04 renyuneyun

I'd be inclined to say that readQuads should be reserved as an internal property and not exposed in the typings.

Do you have a use case for it that cannot be achieved with the match method?

jeswr avatar Apr 15 '22 21:04 jeswr

If it's not underscored, it was considered public. Any negatives to exposing it?

RubenVerborgh avatar Apr 15 '22 21:04 RubenVerborgh

Mainly just to leave the option of changing its behaviour in the future - for instance in work on https://github.com/rdfjs/N3.js/issues/281 or https://github.com/comunica/comunica/issues/594#issuecomment-1099889243, though such issues may not require changes to its behaviour anyway

jeswr avatar Apr 15 '22 21:04 jeswr

Do you have a use case for it that cannot be achieved with the match method?

@jeswr I don't know. It is listed first in the document, so I used it. I could try changing it to match(). But I guess this indicates problems with the document? (The document is not very easy to catch up with for me as a newbie, by the way.)

renyuneyun avatar Apr 15 '22 22:04 renyuneyun

Fair enough - in general I would recommend selecting those methods defined in the DatasetCore definition https://github.com/rdfjs/types/blob/183bda795f57a9464ddf95deac45a0c4a48879cf/dataset.d.ts#L7 wherever possible as this means your app can stay compatible with other dataset implementations as well.

Indeed this may also be a point worth noting in the docs @RubenVerborgh ?

jeswr avatar Apr 15 '22 22:04 jeswr

Mainly just to leave the option of changing its behaviour in the future

In light of the fact that it is mentioned in the documentation, this is a moot point anyway - I now agree that it should be added to the @types/n3 definition.

jeswr avatar Apr 15 '22 23:04 jeswr

Hello, I was going to open a PR but the title of this one seems appropriate.

I was trying to parse an rdfStream.

The README says that you can (line 147):

const parser = new N3.Parser(),
      rdfStream = fs.createReadStream('cartoons.ttl');
parser.parse(rdfStream, console.log);

However when I tried, Typescript complained:

typescript: Argument of type 'ReadStream' is not assignable to parameter of type 'string'

I checked the types and they also don't seem to allow ReadStream.

But then I checked the code and it seems to actually support streams.

So the types don't seem to match the code

Thanks!

andrefs avatar Sep 23 '22 17:09 andrefs

@andrefs Indeed, feel free to PR.

RubenVerborgh avatar Sep 23 '22 19:09 RubenVerborgh

What is needed to update the types? It kinda hard to use the lib in typescript right now; at least for a novice. Another question I had: why is the type Quad_Object used instead of Term at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/n3/index.d.ts#L82 ?

mielvds avatar Feb 09 '23 10:02 mielvds

PR started in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/65275

mielvds avatar Apr 25 '23 08:04 mielvds

@jeswr you were willing to review the PR above?

mielvds avatar May 31 '23 12:05 mielvds

Sure @mielvds - is there a reason it is still in draft mode?

jeswr avatar May 31 '23 13:05 jeswr

no, just me being insecure on what I'm doing :)

mielvds avatar May 31 '23 13:05 mielvds