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

Fix TypeScript types #131

Merged
merged 2 commits into from
Mar 13, 2021
Merged

Fix TypeScript types #131

merged 2 commits into from
Mar 13, 2021

Conversation

thegnuu
Copy link
Contributor

@thegnuu thegnuu commented Mar 1, 2021

Completed was not recognised as a valid type and I was not able to find something within electron or your library which matches this term.

So I added the types manually based on the onCompleted call from index.js.

The current version was not working in my typescript project, at least not without additional changes to add this Completed type.

optimized onCompleted types
@sindresorhus
Copy link
Owner

// @paviro

@paviro
Copy link
Contributor

paviro commented Mar 3, 2021

Will check asap (probably tomorrow).

@paviro
Copy link
Contributor

paviro commented Mar 4, 2021

Okay now I am really confused what I did there.
readonly onTotalProgress?: (file: File) => void; should be readonly onTotalProgress?: (file: Progress) => void;
and readonly onCompleted?: (completed: Completed) => void; should be readonly onCompleted?: (completed: File) => void;
If I am not completely mistaken.

@thegnuu
Copy link
Contributor Author

thegnuu commented Mar 4, 2021

@paviro looking at it now you are right yes, I don't know how I missed the File interface and copied the whole thing. I was not able to test the rest of the interfaces since I already failed on the missing Completed interface.

readonly onCompleted?: (completed: File) => void looks totally fine for me.

readonly onTotalProgress?: (file: Progress) => void;: I guess the param should be renamed like readonly onTotalProgress?: (progress: Progress) => void;, but yes, it should be Progress and not File based on the code.

Do you want me to change it and update the PR or do you want to change it directly within the main branch and close this PR?

@paviro
Copy link
Contributor

paviro commented Mar 4, 2021

I don't really have any experience with typescript which is also why I missed this. So if you can I would be really thankful if you could double check that the other functions I introduced in bd55e77 work as expected when using typescript.

The last question is probably for @sindresorhus as I don't have access to the repo.

@thegnuu
Copy link
Contributor Author

thegnuu commented Mar 4, 2021

I can easily to this if you want! But I guess it will take me a few days, I'm kinda busy atm.

@sindresorhus I don't know how used you are to typescript, if you'd like I can as well rewrite the project to typescript to enable type checking and to allow the automatic generation of types. But I totally understand if you want to stick to js :)

@sindresorhus
Copy link
Owner

I don't know how used you are to typescript, if you'd like I can as well rewrite the project to typescript to enable type checking and to allow the automatic generation of types. But I totally understand if you want to stick to js :)

I have a lot of experience with TS, which is exactly why I prefer to stay with JS.

@thegnuu
Copy link
Contributor Author

thegnuu commented Mar 12, 2021

Totally fine with that. I updated my PR according to the changes discussed.

@sindresorhus sindresorhus changed the title Fix types Fix TypeScript types Mar 13, 2021
@sindresorhus sindresorhus merged commit 2bd888c into sindresorhus:main Mar 13, 2021
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 this pull request may close these issues.

None yet

3 participants