Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type declarations #6

Closed
retog opened this issue Jan 18, 2020 · 6 comments
Closed

Add type declarations #6

retog opened this issue Jan 18, 2020 · 6 comments

Comments

@retog
Copy link

retog commented Jan 18, 2020

A type declaration (index.d.ts) file analogous to https://github.com/rdfjs-base/data-model/blob/master/index.d.ts for this project to be usable in typescript. It is preferable to have the type declarations in the main project rather than relying on an exernal source.

@retog retog mentioned this issue Jan 18, 2020
@tpluscode
Copy link

tpluscode commented Jan 20, 2020

I, and a handful other people, have been busy filling DefinitelyTyped with RDF/JS typings. I would add there too.

@bergos actually did ask for declarations of @rdfjs/namespace to be published as @types/ package too.

It is preferable to have the type declarations in the main project rather than relying on an external source.

TBH, I tend to think otherwise lately. When the libraries are written in JS, it is beneficial that all declarations reside in the DefinitelyTyped repository and can be tested together when something changes in the upstream packages (well, @types/rdf-js mainly, which is the reflection of the specification).

@retog
Copy link
Author

retog commented Jan 20, 2020

@tpluscode I agree that in cases like this where the type declarations refelct the spec rather than the code it makes sense to have them in DefinitelyTyped. Still, I thhink a minimal type declaration (importing rdf-js) in the project would allow the project to be imported without having to add a declaration locally.

@retog
Copy link
Author

retog commented May 8, 2020

@tpluscode I'm having troubles aligning the type declaration @types/rdfjs__dataset, the spec and this implementaton.

  • In the type declaration there is only a 0-Arguments Dataset.dataset() method to create a DatasetCore. I don't find the DatasetCoreFactory interface defined in the spec
  • In this impementation there's a dataset(quads) function which is added to the exported module object alongside the elememts of DatasetCore. So the Dataset exposed here seems to be a merger of the DatasetCoreFactory and DatasetCore and have nothing in common with the `Dataset interface defined in the spec
  • What adds to the confusion but should probably be discussed elsewhere is that all DatasetCore methods return Dataset instances, which basically means one cannot implement DatasetCore without also implementing Dataset which defeats what I think is the purpose of having DatasetCore rather than just Dataset.

Porting this project to TS would ensure consistency between type declaration and implementation. It still should be aligned to the spec.

@tpluscode
Copy link

In the type declaration there is only a 0-Arguments Dataset.dataset() method to create a DatasetCore

Yes, you're right @retog. The type declarations are quite inaccurate.
I've just submitted a fix for that: DefinitelyTyped/DefinitelyTyped#44721

I don't find the DatasetCoreFactory interface defined in the spec

https://rdf.js.org/dataset-spec/#datasetcorefactory-interface

So the Dataset exposed here seems to be a merger of the DatasetCoreFactory and DatasetCore and have nothing in common with the Dataset interface defined in the spec

How so? @rdfjs/data-model implements DataFactory and indeed the exported object is a combination of that and a dataset function which is the DatasetCoreFactory#dataset method

What adds to the confusion but should probably be discussed elsewhere is that all DatasetCore methods return Dataset instances

Mhm, good observation. This should be brought up in https://github.com/rdfjs/dataset-spec/

Porting this project to TS would ensure consistency between type declaration and implementation. It still should be aligned to the spec.

I don't disagree but this pretty much is not going to happen ;)

@retog
Copy link
Author

retog commented May 14, 2020

Thanks for pointer and the fix.

[...]
Mhm, good observation. This should be brought up in https://github.com/rdfjs/dataset-spec/

yes I've seen and commented on rdfjs/dataset-spec#55

Porting this project to TS would ensure consistency between type declaration and implementation. It still should be aligned to the spec.

I don't disagree but this pretty much is not going to happen ;)

You think a PR wouldn't be accepted? Why not? I mean if this project was run by ES-purists there should be no npm-specific stuff and ceratinly no require/module.exports. TS at least generates standard compliant JS, not acceting it in the repo is like only accepting bitmaps but refusing the svgs from which they are generated (which is especially odd if you then need the reconstructed vectors in a sepate repo).

@tpluscode
Copy link

We have agreed to not include typings directly in the rdfjs-base packages (data-model is an exception historically).

Most @rdfjs packages now have types in Definitely Typed (for example @types/rdfjs__dataset.

Please, let's collaborate there on the TS declarations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants