Skip to content
This repository has been archived by the owner on Sep 9, 2021. It is now read-only.

Add missing Key type #68

Closed
wants to merge 4 commits into from
Closed

Add missing Key type #68

wants to merge 4 commits into from

Conversation

robertkiel
Copy link

@robertkiel robertkiel commented Feb 2, 2021

Update

Turned this into a draft as Symbol keys requires Typescript 4.2 which is supposed to be released end ob February.

For more context, see https://stackoverflow.com/questions/59118271/using-symbol-as-object-key-type-in-typescript


Typescript seems to be unable to import under certain conditions types from .js files.

This PR adds the missing type and makes sure that Typescript does not try to import types from .js.

Background

Missing Key type breaks type checking with Typescript on libp2p 0.30.

@welcome
Copy link

welcome bot commented Feb 2, 2021

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines

    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@robertkiel robertkiel marked this pull request as draft February 2, 2021 05:27
@robertkiel robertkiel closed this Feb 2, 2021
@robertkiel robertkiel reopened this Feb 2, 2021
@achingbrain
Copy link
Member

We purposefully generate types from jsdoc comments to mitigate the risk of them becoming out of sync, unless the construct you are trying to type is impossible to represent with jsdoc comments, in which case they go in a .d.ts file.

Typescript seems to be unable to import under certain conditions types from .js files.

This seems to be the actual problem here. What are the conditions under which this stops working?

@robertkiel
Copy link
Author

I am familiar with the concept using JSDoc to generate Typescript types.

The problem here is that Typescript isn't even able to express this:

https://github.com/ipfs/interface-datastore/blob/master/src/key.js#L78-L80

So, the correct behavior for Typescript is to fail on this - independent whether it's a .d.ts file or plain Javascript.

@achingbrain
Copy link
Member

cc @Gozala - any thoughts on this?

get [symbol] is there so we can use it as a crutch during Foo.isFoo type calls, which themselves are a crutch to work around instanceof failing when there are multiple versions of a dep in your node_modules folder.

My gut feeling is all this Foo.isFoo is an antipattern and we should just use instanceof. Obviously it will fail under the above circumstances, but it that's the case you should sort your dependency tree out.

@achingbrain
Copy link
Member

It shouldn't really be loading from /src/types.d.ts at all. The path to the entry file in package.json is dist/src/index.d.ts, and in that directory is types.d.ts with a relative import that should resolve to key.d.ts.

achingbrain added a commit that referenced this pull request Feb 5, 2021
At some point there will be a new aegir that does the copying automatically
but in the interim just copy it on build.

Fixes #68 and #69
achingbrain added a commit that referenced this pull request Feb 5, 2021
At some point there will be a new aegir that does the copying automatically
but in the interim just copy it on build.

Fixes #68 and #69
@Gozala
Copy link
Contributor

Gozala commented Feb 18, 2021

The problem here is that Typescript isn't even able to express this:

https://github.com/ipfs/interface-datastore/blob/master/src/key.js#L78-L80

I do not believe this is accurate, following is valid:

interface Key {
  [Symbol.toStringTag]:"Key"
  //...
}

@Gozala
Copy link
Contributor

Gozala commented Feb 18, 2021

cc @Gozala - any thoughts on this?

I like this changes, generally I prefer coding against interfaces as opposed to concrete implementation / or classes. This pull seems to do that, so I'm in favor

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

Successfully merging this pull request may close these issues.

3 participants