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

Generate type definitions #84

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Generate type definitions #84

merged 1 commit into from
Mar 9, 2021

Conversation

kimdhamilton
Copy link
Contributor

This seemed to be a minimally-invasive way for to get type definitions, as desired by typescript users.

Notes:

  • Based on my searches, publishing to the same dist folder (that webpack minified files are published to) seemed the recommended way, but this is a dark art to me. I didn't want to risk destabilization by modifying the webpack build itself.
  • npm run prepublish output files uploaded here

@kimdhamilton
Copy link
Contributor Author

fyi @dmitrizagidulin

@dmitrizagidulin
Copy link
Contributor

Thank you so much, @kimdhamilton!

@msporny
Copy link
Member

msporny commented Oct 31, 2020

In general, +1 to enabling more communities to collaborate and reuse tooling. Thank you @kimdhamilton!

Ping @OR13 and @tplooker to see if they have any thoughts in this area. I know that @OR13 was struggling w/ TypeScript reuse of some DB libs and had some sort of breakthrough a while ago. I'm unaware of the details there, though.

/cc @dlongley as we've tried really hard to keep things simple and do webpack-able universal node.js/browser libs using the latest ECMA stuff. Don't know if anything we'd do for Typescript would get in the way of keeping the momentum there going... @kimdhamilton's PR looks non-invasive, which is good.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this works for the typescript community, it seems ok to me. Thanks!

@OR13
Copy link
Contributor

OR13 commented Nov 2, 2020

Looks good to me, I am not super familiar with this approach, but I have opened an issue to confirm the behavior in our TS implementation.

@kdenhartog
Copy link

This approach does get the basics right pretty well, but leaves some with a lot to be improved on. For example:

/**
 * Issues a verifiable credential (by taking a base credential document,
 * and adding a digital signature to it).
 *
 * @param {object} [options={}] - The options to use.
 *
 * @param {object} options.credential - Base credential document.
 * @param {LinkedDataSignature} options.suite - Signature suite (with private
 *   key material), passed in to sign().
 *
 * Either pass in a ProofPurpose, or a default one will be created:
 * @param {ProofPurpose} [options.purpose]
 *
 * Other optional params passed to `sign()`:
 * @param {object} [options.documentLoader] - A document loader.
 * @param {object} [options.expansionMap] - An expansion map.
 * @param {boolean} [options.compactProof] - Should the proof be compacted.
 *
 * @throws {Error} If missing required properties.
 *
 * @returns {Promise<VerifiableCredential>} Resolves on completion.
 */
export function issue(options?: {
    credential: object;
    suite: any;
    purpose: any;
    documentLoader: object;
    expansionMap: object;
    compactProof: boolean;
}): Promise<any>;

Being able to further type the suite property to LinkedDataSignature and the return type to Promise<VerifiableCredential> would be very valuable. Additionally, It would be very valuable to check that the types generated here would be compatible with the jsonld types already available in @types/jsonld

As an example the DocumentLoader is defined as such: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/617705ad39c4812b27e611360626d239c4ad5486/types/jsonld/index.d.ts#L21

which seems incompatible with object as the type of documentLoader as defined by this method.

@kimdhamilton
Copy link
Contributor Author

+1 @kdenhartog; my approach doesn't address everything but I was hoping for some start. I'm ok turning this into more of a conversation about which approach we want to use. I hadn't noticed any DB types in DefinitelyTyped, which is the other alternative. But it's more tedious than adding typing at the "source" (and improving it over time). Curious what you and @OR13 think

@kdenhartog
Copy link

Yeah I'd much prefer having typing at the source as well and working from there. I think the main points of consideration would be around whether or not the DB team would want to maintain that work (If they didn't DefinitelyTyped would likely be a better route).

Also someone on our team pointed out that technically the DocumentLoader type is fine because an Function is an object in typescript so it wouldn't break anything. In this case, I think it's fine for us to use your update as a starting point and then to figure out where to go from there.

@OR13
Copy link
Contributor

OR13 commented Dec 7, 2020

I prefer to generate the types along with the code, my experience with manual annotations is that they are often out of date if not added to the CI / release process... wrong types are worse than no types.

@kimdhamilton
Copy link
Contributor Author

I think the main points of consideration would be around whether or not the DB team would want to maintain that work (If they didn't DefinitelyTyped would likely be a better route).

I was hoping that with this approach, it's pretty seamless/easy for DB folks to maintain (it's just part of the build). Then we can help nudge things in the right direction type-wise over time.

But that leads me to the next point. If we like this approach, we could apply it to other commonly-used DB libraries (jsonld, ld-signatures, ...), and build up a good set of types generated at the source (which help keeps them up to date).

@pmcb55
Copy link

pmcb55 commented Mar 9, 2021

Hi guys - I'm just starting to use VC.js, and I'd prefer to start off using TypeScript if possible, so getting this PR merged would be kinda handy. Given it's been approved already, could this be merged any time soon do you think...?

Copy link
Contributor

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dmitrizagidulin dmitrizagidulin merged commit 3afc43e into digitalbazaar:master Mar 9, 2021
@Vinnl
Copy link

Vinnl commented Mar 10, 2021

Hi all, a bit late to the party, and with unsolicited advice, so feel free to ignore. However, I believe there are two significant disadvantages to this approach that you might want to be aware of.

What this does is have TypeScript try to infer the types of the library. This:

  1. Provides a false sense of security: TypeScript can infer quite a lot, but far from everything. This means that while the code will appear to be type safe, it's actually littered with anys that punch a hole through TS's type checking and can have ripple effects throughout one's codebase.
  2. Will cause breaking changes for TypeScript users, e.g. when TS infers other types, or when you decide to directly author the types yourself.

Since consumers of the library can just have the definitions generated themselves relatively easily, I don't think the current approach adds much value either. An alternative and more common approach would be to submit type definitions to DefinitelyTyped, possibly based on the inferred types. That gives downstream users control over breaking changes, and makes it a conscious decision to adopt these types, with the fact that they come from DefinitelyTyped informing them that they could be wrong, incomplete or outdated - and providing a clear path to fixing that themselves.

You could even consider just adding a dependency on those DefinitelyTyped definitions, although that shares some of the risks of the current approach.

Anyway, my 2 cents, feel free to brush aside :)

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

Successfully merging this pull request may close these issues.

None yet

8 participants