-
Notifications
You must be signed in to change notification settings - Fork 181
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(types): fixed tryit type #296
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @DuCanhGH thanks for the PR 🙏 and sorry for the delay 🙃 I really like you're changes to the tryit function, I'd love to get that in. I'm less enthusiastic about the eslint and the other stuff. I'm not against it but I'd like to isolate those changes. If you're still interested in getting this in can you split it into two PRs? One with the tryit changes (which I'd be happy to merge asap) and one with all the other project management type changes. |
@rayepps I've done as requested! Edit: wait, let me revert the dependencies update as well... Edit: done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there are still a lot of code changes outside the tryit interface, are these changes you're wanting because they're important to you or is it just left overs from running eslint?
also, some questions :)
@@ -80,7 +80,7 @@ const memoize = <T>( | |||
* is given previously computed values will be checked | |||
* for expiration before being returned. | |||
*/ | |||
export const memo = <TFunc extends Function>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change from Function
to (...args: any) => any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rayepps ah yes, that was ESLint's complaining. I forgot to change that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rayepps now that I've rechecked that though, I think that it's better that we keep this change to prevent class declarations (due to how we call func - func(...args)
). I can revert it if you want though!
.npmignore
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we deleting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rayepps we already have the field "files"
in package.json, so I figure that this is not needed (when publishing npm will only pick stuffs that match "files"
anyway)
@rayepps yeah they mostly are changes that improve type safety (and nice to have in my opinion). Perhaps I can re-edit the title and description to better describe the PR? Or I can revert them if you want :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it 💪 I like that
- you removed the
as any
cast inside tryit - you got rid of the
UnwrapProsifify
andArgumentsType
generics
Description
This PR fixes
tryit
's type so that it works with parallel and other functions with generics (I noticed that they weren't automatically resolved, the default behavior was to fallback to typeunknown
instead) by declaring two generics instead of one (one is for arguments and the other is for return value) and replacing UnwrapPromisify with Typescript's built-in Awaited.Checklist
package.json
has been bumped (matching semver)yarn build
command has been run and to update thecdn
directory/docs
directory) has been updatedResolves