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

Support for TypeScript #37

Open
tanayseven opened this issue Jan 13, 2021 · 23 comments
Open

Support for TypeScript #37

tanayseven opened this issue Jan 13, 2021 · 23 comments
Labels
enhancement New feature or request

Comments

@tanayseven
Copy link

I really want TypeScript support for Bugout. I'm willing to add it and raise a PR. Do you think I should raise PR in this repo? Or should I raise PR in the Definitely Typed repo?

@chr15m chr15m added the enhancement New feature or request label Jan 15, 2021
@chr15m
Copy link
Owner

chr15m commented Jan 15, 2021

@tanayseven sounds good. What does TypeScript support involve? Can you give me a brief overview of what would need to be added to the codebase?

@tanayseven
Copy link
Author

The major addition would be a type declaration file. <something>.d.ts. Some minor changes would include installation of TypeScript related packages and some typescript related config in the package.json file. The existing code will mostly be untouched.

For more details please refer the documentation

The changes can either be done in two ways. First is in this repo which means that this repo will be self sufficient when it comes to typescript. The other way would be to add the type declarations in the Definitely Typed repo. Which means that library consumer will have to install two separate repositories.

@chr15m
Copy link
Owner

chr15m commented Jan 15, 2021

@tanayseven thank you. Let me have a think about the tradeoffs.

@tanayseven
Copy link
Author

Hey @chr15m, did you get a chance to decide on whether to go ahead or not based on the tradeoffs?

@chr15m
Copy link
Owner

chr15m commented Jan 20, 2021

@tanayseven where can I find in the documentation which packages will need to be added to the package.json file?

@tanayseven
Copy link
Author

@chr15m I think all you need is the typescript package and nothing else. In addition to that there might be a couple more changes to the package.json file. The documentation as you requested

@chr15m
Copy link
Owner

chr15m commented Jan 23, 2021

@tanayseven if it can be done without adding typescript to the packages (only a ...d.ts file) then go ahead and PR to add it here, but if it requires a package many people won't use, then I think it's best to submit to Definitely Typed.

@tanayseven
Copy link
Author

I think it should be possible without the typescript package and just by adding a ...d.ts file and adding a line to package.json. Let me fork the repo and test it on my side. If it works, I'll raise a PR. Till then let's keep this issue open.

@tanayseven
Copy link
Author

I'll be busy for a couple of weeks, requesting you to keep this issue open till I return

@chr15m
Copy link
Owner

chr15m commented Feb 15, 2021

@tanayseven no trouble, happy to leave the ticket open. Thanks for looking at it.

@tanayseven
Copy link
Author

I was trying to add types to my personal project which I will later transfer to the bugout repo.
I was trying to stringify the bugout object using typescript to debug it. But when I do that it gives the following error.

TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'WebTorrent'
    |     property 'torrents' -> object with constructor 'Array'
    |     index 0 -> object with constructor 'Torrent'
    --- property 'client' closes the circle

Is there a cyclic dependency in the implementation of bugout?
If yes, can that be fixed?
I think cyclic dependencies with come back to bite us in the future.

@chr15m
Copy link
Owner

chr15m commented Apr 10, 2021

trying to stringify the bugout object using typescript to debug it

Can you output it to the browser console and inspect it there? The referencing cycle you're referring to appears to be in the WebTorrent library, not Bugout, and I don't think they would consider that a bug.

@tanayseven
Copy link
Author

tanayseven commented Apr 10, 2021

Sorry, I removed that code while trying something else, so I'm unable to reproduce it. As of now assume that issue does not exist. I'll go ahead with adding the type definitions till then.

But as far as I remember, the error looked exactly like the text that I've posted above. Since I was trying it in Svelte.

tanayseven added a commit to tanayseven/bugout that referenced this issue Apr 10, 2021
@tanayseven
Copy link
Author

tanayseven commented Apr 10, 2021

Let me know how I should test it before merging.
I was unable to manually add to my actual project's node_modules and check if it works.

@chr15m
Copy link
Owner

chr15m commented Apr 11, 2021

You might be able to use the npm link command to test it.

@tanayseven
Copy link
Author

Things are a bit stressful in my country, India. It will take a while for me to contribute to this. It's currently hard for me to take time out for doing this.

@chr15m
Copy link
Owner

chr15m commented May 3, 2021

@tanayseven I completely understand and please do not feel any obligation to work on this ticket.

@tanayseven
Copy link
Author

Thanks a lot

@zisra
Copy link

zisra commented Nov 20, 2023

Need this too

@chr15m
Copy link
Owner

chr15m commented Nov 21, 2023

@zisra patches welcome!

@zisra
Copy link

zisra commented Nov 21, 2023

@chr15m there's already an open PR though. There's some things missing to it, should I open a new one?

@chr15m
Copy link
Owner

chr15m commented Nov 21, 2023

@zisra whatever is easiest for you. I know nothing about typescript and I think the original contributor does not have capacity to take it further.

@Kos-M
Copy link
Contributor

Kos-M commented Feb 19, 2024

Typescript is worth the trouble of a migration.
You can indentify lot of issues usually raised in runtime , before even use the lib.

Tried today to start a refactor of the lib that uses typescript.
And found some issues for example in opts argument in Bugout.

In typescript side , whatever we write in the code , it should be defined and makes some meaning somehow , while Js let us to write more arbitary code.

@chr15m tomorrow i ll push the current version in my fork , if you want to elaborate to fix those issues there , and see what im talking about..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants