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

darchtio-srf types are not correctly exported using TS #181

Open
hadar-laguna opened this issue Oct 2, 2024 · 14 comments · May be fixed by #185
Open

darchtio-srf types are not correctly exported using TS #181

hadar-laguna opened this issue Oct 2, 2024 · 14 comments · May be fixed by #185

Comments

@hadar-laguna
Copy link

in the update to 4.5.40 you added the namespace Srf and export = Srf.Srf;
Which caused all of the other types (Dialog, SipResponse, SipRequest, CreateUACOptions and many more)
to not be accessible.

Please fix this.
Thanks!

@davehorton
Copy link
Collaborator

can you refer to the problematic commit that you see?

@danvirsen
Copy link
Contributor

Not OP, but I was in the process of updating drachtio-srf when I noticed this and found that there was an issue created only hours ago.

This is the PR: #176

@hadar-laguna
Copy link
Author

Also, it seems the function "options" is missing from types (srf.options( ...))

@hadar-laguna
Copy link
Author

You should change it to

    export = Srf;

and add options function inside the class Srf.

@danvirsen
Copy link
Contributor

danvirsen commented Oct 3, 2024

I created a new branch in my fork for this.
Could you please help me test? You should be able to just replace the version with the path in your package.json:

{
  "dependencies": {
    "drachtio-srf": "danvirsen/drachtio-srf#bugfix/181"
  }
}

If something is missing, feel free to make changes yourself.
A lot of methods have multiple overloads so it's completely possible that a couple are still missing.

I haven't checked out or added the "options" method yet.

https://github.com/danvirsen/drachtio-srf/tree/bugfix/181

@danvirsen
Copy link
Contributor

I have added all missing (I hope) middleware methods for "srf".

@hadar-laguna
Copy link
Author

Hey, So i checked it, and its still problematic since the typing is kind of "lying" as to the whole class.
export = Srf;
is not necessarily the solution, i don't have a lot of time, i will try to fix it later this week and update you, or someone else can try it!

@danvirsen
Copy link
Contributor

Yes, that's true. I did the changes blind but when I tested my changes I did get some "implicit any type" errors and other warnings that some types weren't available. So things weren't exposed correctly.
I have adressed that now (I think) and pushed the changes, and my application is at least happy again.

The codebase is JavaSCript and since the types are managed manually they will always "lie" I suppose. They're not connected to the actual code in any way which make some things that aren't documented lag behind and only surface when someone wants to use a specific call or function/method.

It's a risk to have manually handled types in the repository. Intellisense will only show what the types contain, which like I said aren't actually connected to the code, so there's still the risk of runtime errors due to the types being wrong.

I'm not sure if there is a good way to try and automate this.

One solution might be to simply remove the types, but they do contain a couple of ondocumented ways to call certain methods so I do think they add some value.

@hadar-laguna
Copy link
Author

looks good!
working for me as such:

import * as Srf from 'drachtio-srf';
import { CreateUACOptions, Dialog, SrfRequest, SrfResponse } from 'drachtio-srf';

  // eslint-disable-next-line
  // @ts-ignore
const srf: Srf.default = new Srf();

its not the most typescripty but it works..

@danvirsen
Copy link
Contributor

danvirsen commented Oct 6, 2024

Since the default export is the class Srf, I use it like this:

import Srf, { CreateUACOptions, Dialog, SrfRequest, SrfResponse } from "drachtio-srf";
const srf = new Srf();

@danvirsen
Copy link
Contributor

danvirsen commented Oct 6, 2024

I was thinking that it might make sense to move the types to the DefinitelyTyped project instead of having them in this repository.

https://github.com/DefinitelyTyped/DefinitelyTyped

Until then hopefully my PR fixes the issues we are having for now.

orgads added a commit to audiocodes/drachtio-srf that referenced this issue Oct 31, 2024
@orgads orgads linked a pull request Oct 31, 2024 that will close this issue
@orgads
Copy link
Contributor

orgads commented Oct 31, 2024

Proposed fix:

@danvirsen
Copy link
Contributor

I have added a comment to your PR but overall I think it looks good.
The bigger question still remains for me though: Should the types be in this repository at all?

I think the best thing would be to add drachtio-srf to DefinitelyTyped and remove the types from this repository. That way Dave and the other maintainers don't need to be bothered with TypeScript things (this is a JavaScript project after all), and fixes to the types themselves doesn't require getting a PR approved and waiting for a new release of the package.

Unfortunately I don't have time in the nearest future to create a valid test suite for drachtio-srf in the DefinitelyTyped project. I think that would be the best solution if anyone has the opportunity to do that.

orgads added a commit to audiocodes/drachtio-srf that referenced this issue Oct 31, 2024
@orgads
Copy link
Contributor

orgads commented Oct 31, 2024

@davehorton what do you prefer? Keep the types here or create a package in DefinitelyTyped?

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