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

✨ improvements + TypeScript Types #50

Merged
merged 4 commits into from Jan 7, 2021
Merged

✨ improvements + TypeScript Types #50

merged 4 commits into from Jan 7, 2021

Conversation

harshzalavadiya
Copy link
Contributor

@harshzalavadiya harshzalavadiya commented Jan 5, 2021

Split from #46

Changelog

  • added .gitignore
  • moved History.md to CHANGELOG.md
  • added TypeScript types

@harshzalavadiya harshzalavadiya changed the title 📝 improvements ✨ improvements Jan 5, 2021
@harshzalavadiya harshzalavadiya changed the title ✨ improvements ✨ improvements + TypeScript Types Jan 5, 2021
Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@harshzalavadiya thank you! I have just few questions, please see my comments

README.md Outdated
Comment on lines 3 to 4
[![GitHub Actions Status](https://github.com/Gozala/querystring/workflows/CI/badge.svg)](https://github.com/Gozala/querystring/actions)
[![GitHub Legacy Actions Status](https://github.com/Gozala/querystring/workflows/LEGACY/badge.svg)](https://github.com/Gozala/querystring/actions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this belongs to PR which configures the actions (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this was supposed to be in GitHub Actions branch but since Readmd.md is changed to README.md in this one I thought of adding it here so there can be minimal diff, so first CI branch needs to get merged I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let's put this one on hold then for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

index.d.ts Outdated
@@ -0,0 +1,17 @@
type decodeFuncType = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Project exports three modules. Will querystring/encode and querystring/decode be also covered with this typings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't earlier but I have made change to accomodate that

Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @harshzalavadiya ! Please see my comments

@@ -0,0 +1,20 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to keep typings in e.g. typings folder?

It'll be great to not mix them with regular modules (same as e.g. we keep tests in test folder) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that, by specifying "typings" flag in package.json but seems it's not working also we have seprate file based import as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

also we have separate file based import as well

What exactly do you mean by that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

means you can use same function multiple ways

import qs from "querystring"
import encode from "querystring/encode"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, there's seems no way to keep then in dedicated dir. It's a pity, anyway let's go with this

CHANGELOG.md Outdated

## [Unreleased]

## [0.2.1] - 2021-01-02
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove release notes for new version (I think it's nice to have releases not mixed with PR's that address different concerns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @harshzalavadiya !

@@ -0,0 +1,20 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, there's seems no way to keep then in dedicated dir. It's a pity, anyway let's go with this

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

2 participants