Skip to content

✨ improvements + TypeScript Types - #50

Merged
medikoo merged 4 commits into
Gozala:masterfrom
harshzalavadiya:master
Jan 7, 2021
Merged

medikoo merged 4 commits into
Gozala:masterfrom
harshzalavadiya:master

Conversation

@harshzalavadiya

@harshzalavadiya harshzalavadiya commented Jan 5, 2021 •

Copy link
Copy Markdown
Contributor

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

@medikoo medikoo left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread index.d.ts Outdated
@@ -0,0 +1,17 @@
type decodeFuncType = (

Copy link
Copy Markdown
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
Copy Markdown
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

@medikoo medikoo left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread decode.d.ts
@@ -0,0 +1,20 @@
/**

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@harshzalavadiya harshzalavadiya Jan 6, 2021 •

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread CHANGELOG.md Outdated

## [Unreleased]

## [0.2.1] - 2021-01-02

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@medikoo medikoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @harshzalavadiya !

Comment thread decode.d.ts
@@ -0,0 +1,20 @@
/**

Copy link
Copy Markdown
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.

2 participants