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

Types for better intellisense #14

Merged
merged 7 commits into from
Oct 19, 2022
Merged

Conversation

TigranKirakosov
Copy link
Contributor

This is somewhat naive attempt to provide intellisense with JSDoc
Please help with review and suggesting types and stuff

Maybe it is possible to even use TypeScript instead of JSDoc?

@TigranKirakosov TigranKirakosov marked this pull request as draft October 12, 2022 14:42
@tysky
Copy link
Contributor

tysky commented Oct 12, 2022

Maybe it is possible to even use TypeScript instead of JSDoc?

Good idea, I'm totally for it. But I think we need to discuss it with other teammates.

src/multipleObserver.js Outdated Show resolved Hide resolved
@TigranKirakosov TigranKirakosov marked this pull request as ready for review October 13, 2022 19:46
src/common.d.ts Outdated Show resolved Hide resolved
src/common.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@nulldef nulldef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool work, looks good to me 👍

@TigranKirakosov
Copy link
Contributor Author

I assume changes require to patch package version, set it to 1.4.2

Is there anything that should be done before merge?

@nulldef
Copy link
Member

nulldef commented Oct 17, 2022

I assume changes require to patch package version, set it to 1.4.2

Yeah, it should be fine.

Is there anything that should be done before merge?

Before merging – no (as far as I remember), but the package needs to be re-bundled before publishing.

@TigranKirakosov
Copy link
Contributor Author

Well, as I'm able to, gonna merge then 😎

@TigranKirakosov TigranKirakosov merged commit bc1835b into master Oct 19, 2022
@TigranKirakosov TigranKirakosov deleted the feature/jsdoc-type-coverage branch October 19, 2022 15:01
@TigranKirakosov
Copy link
Contributor Author

TigranKirakosov commented Oct 19, 2022

It's weird that after merge was done coverage badge value somehow has been decreased despite nothing did indicate or warn :(
Or maybe its just expected behavior?

@nulldef
Copy link
Member

nulldef commented Oct 19, 2022

It's weird that after merge was done coverage badge value somehow has been decreased despite nothing did indicate or warn :(
Or maybe its just expected behavior?

All the jobs are completed successfully, and a coverage report shows us 100%, so I guess it's a coveralls issue.
Perhaps it's better to move to GitHub Actions coverage check (if they support that).
Also, the reason might be outdated packages (it definitely should be fixed).

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.

3 participants