Skip to content

Add TypeScript typings#14

Closed
eioo wants to merge 3 commits intoKequc:masterfrom
eioo:master
Closed

Add TypeScript typings#14
eioo wants to merge 3 commits intoKequc:masterfrom
eioo:master

Conversation

@eioo
Copy link

@eioo eioo commented Jul 14, 2019

Resolves #13
TypeScript typings would be nice for this package. I personally try avoiding "any" as much as I can. It's also nice to get autocompletion for the new options this package offers.

index.d.ts Outdated

interface IKnexStringCaseConfig extends Knex.Config {
appStringcase?: StringCase | StringCase[];
dbStringcase?: StringCase | StringCase[];
Copy link
Owner

Choose a reason for hiding this comment

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

The two parameters appStringcase and dbStringcase can also include functions.

@Kequc
Copy link
Owner

Kequc commented Jul 14, 2019

In general I'm not happy with adding typescript definitions with this implementation because it means at least one additional dependency that only serves typescript users. The utiliy of this change seems fairly inconsequential because this library is essentially just doing configuration.

What I mean is, that while I understand that it's nice to have thorough autocompletion. It isn't needed when it's only used the first time you implement knex-stringcase into your configuration. And even then only if you are using typescript.

Is there a way to have a much looser implementation that doesn't force dependencies?

@eioo
Copy link
Author

eioo commented Jul 15, 2019

You're right. The implementation was pretty bulky because of the dependency, the types package was also depending on knex package itself. @types/knex seems to be unmaintained anyways.

I made knexStringcase() generic and added usage example for it in README.md, let me know what you think.

Make stringcase type dynamic

Add TypeScript usage
@Kequc
Copy link
Owner

Kequc commented Jul 15, 2019

That's a lot better thank you. I believe parameters appStringcase and dbStringcase are still incorrect as it can be a string, or a function or an array of strings, or an array of functions, or a mix of both.

@eioo
Copy link
Author

eioo commented Jul 15, 2019

That should do it. "Stringcase" and "StringcaseFn" probably could have better names but didn't came up with any. But hey, now it works! 👍

@Kequc
Copy link
Owner

Kequc commented Aug 23, 2019

I've decided not to support typescript on this project it's unnecessary.

@Kequc Kequc closed this Aug 23, 2019
@shellscape
Copy link

shellscape commented Sep 4, 2021

Having a contributor go through all of those steps just to decide at the very end that you're not going to accept their work is a really bad look. At the very least you should have given them a detailed apology in return for their time.

@eioo thank you for your work on this. I'm going to put it up on a public gist to preserve it in the event your fork goes away one day. solid work.

@Kequc
Copy link
Owner

Kequc commented Sep 8, 2021

It has remained here for over a year why would the fork go away. How cordial do I need to be in order not to get hounded by Typescript users. I think I explained in quite a lot of detail why it is unnecessary.

You're trying to guilt me and if I'm completely honest you're baiting me, why is that a good look and not how I cared for this merge request?

If this feature is updated to use the newest patch version I'll merge it.

@Kequc Kequc reopened this Sep 8, 2021
@shellscape
Copy link

Maybe something is lost in translation on this one - it doesn't appear that you receive critical feedback well. If you perceive criticism regarding contributors and your behavior towards them as guilting, baiting, etc then open source may not be for you. You don't have to agree with feedback, but to suggest something so dramatic as "baiting" is juvenile.

Regardless, I'm disengaging from this project.

@Kequc
Copy link
Owner

Kequc commented Sep 11, 2021

Thank you for your critical feedback, which was to pamper people with pleasantries. It's ok if you call repository owners juvenile though.

@Kequc Kequc closed this Sep 11, 2021
@acro5piano
Copy link

I'm really glad to see the migration to TypeScript on v1.5.0. Nice work! Thank you @Kequc

@Kequc
Copy link
Owner

Kequc commented Jul 14, 2025

I came to realize Typescript should basically just be everywhere. Node has recently realized this too now Typescript seems to be supported out of the box. Unfortunately there's issues here with the import that I was not able to fix, but I'm sure we'll figure it out in time.

Might be just a named export instead of a default export is the way to go with libraries like this. Thanks!

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.

Create TypeScript definitions

4 participants