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

The library is difficult to use with a strict TypeScript configuration #26

Closed
t-richards opened this issue Aug 25, 2020 · 6 comments · Fixed by #25
Closed

The library is difficult to use with a strict TypeScript configuration #26

t-richards opened this issue Aug 25, 2020 · 6 comments · Fixed by #25

Comments

@t-richards
Copy link
Contributor

t-richards commented Aug 25, 2020

I have the following TypeScript code in my own project:

// File: src/main.ts

import { CometD } from 'cometd';
import { adapt } from 'cometd-nodejs-client';
adapt();

const client = new CometD();

And the following TypeScript configuration (notably the "strict" and "noImplicitAny" options):

// File: tsconfig.json
{
    "include": [
        "src/**/*"
    ],
    "compilerOptions": {
        "target": "es2017",
        "module": "commonjs",
        "baseUrl": "./src",
        "noEmit": true,
        "strict": true,
        "noImplicitAny": true,
        "esModuleInterop": true
    }
}

Given the above two things, attempting to call the adapt() function from this library results in a build error.

> tsc

src/main.ts:4:23 - error TS7016: Could not find a declaration file for module 'cometd-nodejs-client'. '/node_modules/cometd-nodejs-client/cometd-nodejs-client.js' implicitly has an 'any' type.
  Try `npm install @types/cometd-nodejs-client` if it exists or add a new declaration (.d.ts) file containing `declare module 'cometd-nodejs-client';`

4 import { adapt } from 'cometd-nodejs-client';
                        ~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.

In my opinion, the ideal solution to this problem would be for this library to provide a "TypeScript Declaration File" (.d.ts), in which exported functions and types are explicitly declared for use within TypeScript.

Is this something the project considers to be desirable? If so, please see #25 for a proposed solution.

@sbordet
Copy link
Member

sbordet commented Aug 25, 2020

What does declare module 'cometd-nodejs-client'; do?
Would it be a one-liner that fixes your problem?

@t-richards
Copy link
Contributor Author

Yes, while that would technically solve the build error, it does so in a way that I don't consider to be useful.

If I were to only specify declare module 'cometd-nodejs-client';, now I am free to make the following (admittedly contrived) mistake which will result in a hard-to-debug handshake error at runtime:

import { CometD } from 'cometd';
import { adapt } from 'cometd-nodejs-client';
adapt({ httpProxy: { uri: true, includes: ['example.com'] } });

const client = new CometD();
client.configure({ url: 'https://example.com' });
client.handshake((h) => {
  if (!h.successful) {
    console.error('Handshake was not successful');
  }
});

The handshake does not fail because of the inability to connect to example.com, instead, it actually fails because of the inability to parse the proxy URI, true.

This is precisely the kind of error which I expect TypeScript to catch for me ahead of time, prior to running my code.

@sbordet sbordet linked a pull request Aug 25, 2020 that will close this issue
@sbordet
Copy link
Member

sbordet commented Aug 27, 2020

@t-richards thanks for the explanations! Turns out that a major user of this library was also using TS and had an almost identical type definition, so I'm positive about merging your contribution. See PR for additional comments.

@sbordet
Copy link
Member

sbordet commented Aug 27, 2020

@t-richards let me know if you're interested in contributing something similar for https://github.com/cometd/cometd-nodejs-server 😃

@alexanderkjeldaas
Copy link

will this be merged?

@sbordet
Copy link
Member

sbordet commented Dec 14, 2020

@alexanderkjeldaas yes it will.

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 a pull request may close this issue.

3 participants