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

Typescript type definitions #431

Closed
DanielRose opened this issue Jul 6, 2018 · 24 comments
Closed

Typescript type definitions #431

DanielRose opened this issue Jul 6, 2018 · 24 comments

Comments

@DanielRose
Copy link

I've started with Typescript type definitions for some of the socketcluster libraries. They are currently in my private repository and incomplete. Is there any interest in this?

@Crypto-Q
Copy link

Yeah would nice to see this public thank you.

@peterkov
Copy link

peterkov commented Aug 1, 2018

Very much so! We have also made private type definitions for our internal use case but they are also incomplete. Would be great to finally see a public type definition. I'm up to help out with PRs later.

@DanielRose
Copy link
Author

My current state is at https://github.com/DanielRose/DefinitelyTyped/tree/socketcluster There are type definitions for sc-auth, socketcluster, socketcluster-client and socketcluster-server. It is still incomplete, especially missing tests (except for sc-auth).

@jondubois
Copy link
Member

@DanielRose We should link to it from the project website :)
Do you intend to do a PR on the main DefinitelyTyped repo?

@DanielRose
Copy link
Author

@jondubois Once it is in a somewhat releasable state, I'll send a PR. I'm currently expanding the code by adding expirymanager, fleximap, sc-broker, sc-broker-cluster and adding additional methods and so on.

@DanielRose
Copy link
Author

I now have a version that throws no errors when compiling for DefinitelyTyped. I have the type definitions (though incomplete in some places) for:

  • sc-auth
  • socketcluster
  • socketcluster-client
  • socketcluster-server
  • expirymanager
  • fleximap
  • sc-broker
  • sc-broker-cluster
  • sc-channel

Please take a look. If nobody complains I'll send a PR in the next days.

@Akuukis
Copy link

Akuukis commented Sep 19, 2018

@DanielRose thanks for types! I tried it out like this:

  1. Add type roots to tsconfig
...
        "typeRoots": [
            "../../node_modules/@types/",
            "node_modules/@types/",
            "src/types/"
        ]
...
  1. git clone your repo, copy the following to "src/types", install external types and delete tests from them
 yarn add socketcluster-server socketcluster-client
 mkdir src/types
 cp -r ../../../DefinitelyTyped/types/socketcluster src/types/
 cp -r ../../../DefinitelyTyped/types/sc-broker-cluster src/types/
 cp -r ../../../DefinitelyTyped/types/sc-broker src/types/
 cp -r ../../../DefinitelyTyped/types/fleximap src/types/
 cp -r ../../../DefinitelyTyped/types/socketcluster-server src/types/
 cp -r ../../../DefinitelyTyped/types/sc-broker-cluster src/types/
 cp -r ../../../DefinitelyTyped/types/expirymanager src/types/
 cp -r ../../../DefinitelyTyped/types/sc-auth src/types/
 cp -r ../../../DefinitelyTyped/types/sc-channel src/types/
 yarn add -D @types/expirymanager @types/jsonwebtoken @types/jsonwebtoken @types/async @types/component-emitter @types/ws
  1. No TS type errors in typing directors, so far so good.
  2. back in my code types are not found in neither form
import * as socketClusterServer from 'socketcluster-server'
// OR
import socketClusterServer = require('socketcluster-server');

I am not sure whether problem is in setup of DefinitelyTyped or types themselves, so here's the steps to reproduce and please let me know!

@Akuukis
Copy link

Akuukis commented Sep 19, 2018

I copied socketcluster-server/*.d.ts into ./node_modules/socketcluster-server and it is found now.

But I have a error in SocketCluster's own example

import * as socketClusterServer from 'socketcluster-server'

... do express stuff, create httpServer ...

const scServer = socketClusterServer.attach(httpServer);
//                                   ^

[ts] Property 'attach' does not exist on type 'typeof import("node_modules/socketcluster-server/index")'.

@DanielRose
Copy link
Author

DanielRose commented Sep 20, 2018

@Akuukis typeRoots is deprecated. It is a relic from TypeScript v1 (or something like that) and doesn't work with modern type definitions. The correct way to add custom type definitions is via the combination of baseUrl and paths, i.e.

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "*": ["src/types/*"]
    }
  }
}

Note that by convention, the type definitions in node_modules/@types and those placed next to the js-files are always loaded.

I've updated the type definitions to add attach and listen.

@Akuukis
Copy link

Akuukis commented Sep 20, 2018

@DanielRose Thanks, that worked.

Types looks good so far. Can you please also type sc-uws? E.g. in Webpack's case it should be imported separately and passed as option.

@DanielRose
Copy link
Author

sc-uws is basically the same as uws. Unfortunately the type definitions there are not up to date. I could work on them later, but first I would want to finish these.

@Akuukis
Copy link

Akuukis commented Sep 24, 2018

@DanielRose Right, sc-uws can be done seperately. In that case types looks good to me

@jondubois
Copy link
Member

@DanielRose My current stance is that we shouldn't force developers to use TypeScript in order to use SocketCluster but also we don't want to discourage them from using it either.

Would having all the type definitions in a folder directly inside this socketcluster repo make them more accessible and easier to maintain (instead of keeping them in the DefinitelyTyped repo)? What is the common practice in the TypeScript community?

@Akuukis
Copy link

Akuukis commented Sep 27, 2018

I have seen three cases:

  1. DefinitelyTyped repo, that follows versioning at major and minor level
  2. bundled types inside repo, and also a stub DefinitelyTyped repo that throws warning about being only a stub
  3. is written in TypeScript itself, but otherwise the same as No.2

Personally, when doing yarn add something I automatically do also yarn add -D @types/something and just press CTRL+C if that throws warning about being a stub. In such case either way is equally accessible.

@jondubois
Copy link
Member

@Akuukis the second approach sounds good. I think that if we have the types in the main socketcluster repo, they'll be more likely to be kept up to date with the JS code. Also having a stub with warnings in DefinitelyTyped repo sounds good.

@DanielRose
Copy link
Author

Leaving aside how to get the .d.ts files (either autogenerated because the code is written in Typescript, or manually written), there are two main options, as @Akuukis wrote:

  1. Include them as part of the bundle (ex. moment)
  2. Place them in DefinitelyTyped (ex. express)

Advantage of bundling the type definitions:

  • Developers see them directly, and can update them, if necessary
  • More likely that they will get updated

Disadvantages:

  • Updating them means a new library release
  • They get distributed to everyone (i.e. not only developers using Typescript)
  • A Microsoft employee looks at updates and (especially) new type definitions in DefinitelyTyped, but not somewhere else

@jondubois
Copy link
Member

@DanielRose we can go with whatever you think is best ;p
I guess SC types aren't likely to change too much in the future.

We can mention the existence of the typescript definitions on the socketcluster.io website when ready.

@pscanlon1
Copy link

Sorry for ignorance... But could you provide how to get these types? Im not seeing them in any of the source. Thanks!

@pscanlon1
Copy link

apologies, I found the types, SCServer import worked when I threw them all in the node_modules/@types directory... But SCWorker is not?

@Akuukis
Copy link

Akuukis commented Oct 15, 2018

bump.

@jondubois , what's the checklist to get types into the repo?

@jondubois
Copy link
Member

jondubois commented Oct 21, 2018

@Akuukis I haven'ted used SC with TypeScript yet so I'm not sure what steps are involved so feel free to lead the way if you have something specific in mind. You can make a proposal here or as a new issue and if/when the community is generally happy with the proposal then you can make a PR.

@DanielRose
Copy link
Author

Sorry I've been absent for so long. I was on vacation, and then we had major production issues (hangs and crashes). I'll try to finish the PR in the next couple of days.

@DanielRose
Copy link
Author

Finally got everything done. Found a few places where I had wrong definitions or was missing them: DefinitelyTyped/DefinitelyTyped#30944

@DanielRose
Copy link
Author

PR got merged!

https://www.npmjs.com/package/@types/socketcluster

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

6 participants