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

Added a typescript defs file #5

Merged
merged 4 commits into from
Jan 11, 2019
Merged

Added a typescript defs file #5

merged 4 commits into from
Jan 11, 2019

Conversation

maraisr
Copy link
Contributor

@maraisr maraisr commented Jan 11, 2019

Not sure we need all the fancy things from that @types/classnames repo - but here is something nice and tight.

If you're not too keen; happy to amend this with a simple copy-paste from their one, and rename somethings.

resolves #3

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

@kentcdodds looks good to me, how about to you?

Copy link

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Once we fix the exports this'll be ready.

Thanks for considering including this @lukeed!

We'll also need to add index.d.ts to the package.json files property.

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

Aye, I'll take care of properly distributing the types to npm – that's about all I can contribute here.

As per Twitter thread, I can't really weigh in on the right/wrong/better ways for typings, so once you all come to an agreement I'll merge.


I will say that multiple of my repos have been PR'd with export = lib. @marvinhagemeister shared this link with me about it.

@kentcdodds
Copy link

I'm pretty new to TS, but from what I understand export = is an old way to do things that pre-dates the ESModules specification. In my own version of these definitions, I'm using export default and that's working well 👍 (plus export {ClassArray, ClassDictionary, ClassValue})

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

Ah, then I suppose the above was specific to kleur since it's Node/CommonJS only & can be required as default or partially.

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

Is this final form?

export type ClassValue = ClassArray | ClassDictionary | string | number | null | boolean;

export interface ClassDictionary {
	[id: string]: any;
}

export interface ClassArray extends Array<ClassValue> { }

declare const clsx: (...classes: ClassValue[]) => string;

export default clsx;

@kentcdodds
Copy link

Yep, I think that'll do it!

@marvinhagemeister
Copy link

marvinhagemeister commented Jan 11, 2019

yup, the export = foo way is only needed when the library doesn't use proper exports and uses plain CommonJS style exports (module.exports = foo) instead. The only exception is when the special __esModule property is defined on exports:

// Note: exports === module.exports 
Object.defineProperty(exports, "__esModule", { value: true });

In the case for clsx the proper way to do it is via a export default expression as already suggested 👍

@kentcdodds
Copy link

Thanks for the help @marvinhagemeister!

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

Thank you @marvinhagemeister, makes sense~!

And thank you all, especially @maraisr, for the types. Since they are so simple & small, I'll just add them here after all. Patch incoming~

@lukeed lukeed merged commit 08d028f into lukeed:master Jan 11, 2019
@kentcdodds
Copy link

👏

@lukeed
Copy link
Owner

lukeed commented Jan 11, 2019

Released in 1.0.1 🙌

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.

TypeScript support
5 participants