Add TypeScript declaration. #103

bradleyayers · 2016-11-09T11:00:37Z

This resolves #102

bradleyayers · 2016-11-15T19:34:07Z

Any further thoughts on this @JedWatson?

cwmacdon · 2016-12-08T19:43:00Z

It would be nice if there were definitions for the alternate classnames/dedupe and classnames/bind versions.

How does this look for bind.d.ts?

import { default as base } from './index';

interface IClassNamesBind {
    bind: (styles: any) => typeof base;
}

declare var classNames: IClassNamesBind;

export default classNames;

bradleyayers · 2016-12-08T21:04:40Z

@cwmacdon looks good to me — I'd love to see that get merged in too 👍.

jednano · 2016-12-10T05:56:42Z

@bradleyayers is there any reason these definitions don't match the ones on DefinitelyTyped 100%?

Also, the publishing doc states the following:

Also note that if your main declaration file is named index.d.ts and lives at the root of the package (next to index.js) you do not need to mark the "types" property, though it is advisable to do so.

Perhaps you can add it?

{
    "types": "./index.d.ts"
}

bradleyayers · 2016-12-11T04:15:49Z

@jedmao I'm not sure I considered just copying over the @types/ code, maybe I wanted to avoid licensing issues, but using it (or matching it) indeed seems more desirable than having a set of deviating definitions.

Having the .d.ts placed next to the index.js should indeed work. I'll give that a go and update the PR

jednano · 2016-12-11T04:18:19Z

It will work, but they still recommend being explicit about it. I think what i was trying to say before its that if what you have here is better than the one on DT, perhaps you should submit a PR to DT first so that they are in sync. If you do so, submit to the types-2.0 branch.

bradleyayers · 2016-12-11T10:18:26Z

I'll compare and take the best and upstream if necessary. Thanks

cwmacdon · 2017-01-05T19:10:31Z

Any progress on this @bradleyayers @jedmao? Anything I can do?

bradleyayers · 2017-01-06T01:27:02Z

@cwmacdon I haven't had time to get back to this. But what you could do is to look into whether we can just copy the DefinitionTyped definitions into classnames directly (I'm not sure if there's licensing issues). This would avoid having two different definitions.

If we can, raise a PR to do so 😄

cwmacdon · 2017-01-12T18:29:43Z

@bradleyayers So I tried copying the Definitely Typed version into my node_modules/classnames/index.d.ts

Then I wrote a file to use it

import classNames from 'classNames';

and it gave the error
File {.../classNames/index.d.ts} is not a module.

The definition that you have included above works perfectly.

@adidahiya essentially wrote the DefinitelyTyped version, maybe he can weigh in?

adidahiya · 2017-01-12T18:56:46Z

there are no licensing issues. I think you are free to copy the definitions I wrote for DefinitelyTyped, just make sure to add the package to notNeededPackages.json. I'll take a look at the code here now

adidahiya · 2017-01-12T19:00:13Z

index.d.ts

@@ -0,0 +1,9 @@
+type ClassValue = string | number | ClassDictionary | ClassArray;


please make this match the DT typings. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a8dc31d5a2a55bf458834680d4653bc452949d84/classnames/index.d.ts

adidahiya · 2017-01-12T19:00:39Z

index.d.ts

+type ClassValue = string | number | ClassDictionary | ClassArray;
+
+interface ClassDictionary {
+	[id: string]: boolean;


same here, this needs to allow undefined and null. also, personal preference (generally in the TS community), can you use 4-space indentation instead of tabs?

adidahiya · 2017-01-12T19:05:16Z

index.d.ts

+
+interface ClassArray extends Array<ClassValue> { }
+
+export default function(...classes: ClassValue[]): string;


most consumers are currently doing import * as classNames from "classnames";, and this will break that. actually I don't even think this is the shape of the export; see this part of the DT readme for guidance.

$ node -p 'require("classnames").default' undefined $ node -p 'require("classnames")' [Function: classNames]

you should use the export = syntax from my DT typings instead.

@adidahiya There are many ways to do this...
import classNames from 'classNames';
import * as classNames from 'classNames';
import classNames = require('classNames');

Is there a "best" way? Is there a way to write the definition to work with all of the above?

both import classNames from 'classNames'; and import * as classNames from 'classNames'; are generally OK. The last one that uses require() is discouraged because it is old CommonJS-style syntax.

but, like I said, the DefinitelyTyped README stipulates that you should not declare a default export unless require("classnames").default actually returns the module.

@adidahiya Thanks for your input here. Good to know.

Looking at your definition on DefinitelyTyped, what is the purpose of the classnames module wrapped around the export?

declare module "classnames" { export = classNames }

I think this is the underlying reason why @bradleyayers and myself strayed from using the definitions on DefinitelyTyped? If we remove this wrapping module, then we can just import * as classNames from 'classNames';

Ah, you don't need that part. That was a remnant from before we had @types on NPM -- when you use those typings through tsd or the typings tool, you would need the ambient declaration of the "classnames" module. Now that we have node-style resolution of index.d.ts, that part isn't needed.

@adidahiya Would you recommend updating the DefinitelyTyped definition to remove it as well? Or should we just leave it out in this PR?

Yeah, updating the DT definition sounds like a good idea now that the repo has switched over to @types completely. Once a release of classnames is made that includes definitions though, we should remove those typings from DT (and update notNeededPackages.json in the same PR).

cwmacdon · 2017-01-12T20:12:05Z

@bradleyayers Thanks to input from @adidahiya and @jedmao, Could you update the PR with the changes outlined below?

index.d.ts with the definition from DefinitelyTyped with one slight change: Remove the declare module "classnames" wrapper around the export.
Add a bind.d.ts with following:

import * as base from './index';

interface IClassNamesBind {
    bind: (styles: any) => typeof base;
}

declare var classNames: IClassNamesBind;

export = classNames;

Change package.json to explicitly point to the type definitions:

    "types": "./index.d.ts"

Then once this is complete and merged, a PR to DefinitelyTyped will be needed to add classNames to the notNeededPackages.json to help users know that the types are included in this package now.

…ind.d.ts

cwmacdon · 2017-02-15T16:57:18Z

@JedWatson Any chance a new release could be cut with this PR?

cwmacdon · 2017-02-21T16:51:44Z

@dcousens Perhaps you could merge this PR? I see you've kind of taken over merging PRs since mid last year.

dcousens · 2017-02-22T01:07:15Z

I'm waiting on @JedWatson to add me as a contributor for publishing to npm 👍

adidahiya · 2017-03-02T18:52:24Z

bump again here @JedWatson @dcousens, thanks

Havret · 2017-04-17T19:07:21Z

It would be great if it was merged. What are we waiting for?

adidahiya · 2017-05-15T17:25:14Z

This PR just merged in DT so you should be able to pick up the latest @types/classnames with most of these improvements. DefinitelyTyped/DefinitelyTyped#16420

cwmacdon · 2017-05-15T17:30:22Z

@adidahiya Thanks for the heads up. I have doubts this will ever get merged in here, so @types/classnames will be the best we can get.

maciejw · 2018-02-12T16:47:06Z

@bradleyayers can you tell me why you decided to go with export = classNames; instead of export default, I'm curious

this does not work with allowSyntheticDefaultImports = false witch is default setting in ts.

webpack can handle this when I do

import  * as classNames from 'classnames'

but rollup will say

Cannot call a namespace ('classNames')

with allowSyntheticDefaultImports = true both works fine using this

import  classNames from 'classnames'

export = has many issues and it requires this kind of import

import classNames = require('classnames')

(https://www.typescriptlang.org/docs/handbook/modules.html)

OMG import default does not work with jest and wallaby :) I'm going to kill myself :)

adidahiya · 2018-02-12T17:07:05Z

@maciejw you probably want the new --esModuleInterop flag added in TS 2.7.

this does not work with allowSyntheticDefaultImports = false witch is default setting in ts.

Note that this only affects type checking, not JS emit.

See the DefinitelyTyped README for info about why export = syntax is used here.

maciejw · 2018-02-12T17:50:07Z

@adidahiya indeed I have ts 2.7, but I did not change this flag... this esModuleInterop flag break my postcss modules in jest. witch worries me gravely:)

now, what I did is I copied d.ts locally and declared it as another module and that imported types only
to a file and reexported it as below. I had to copy d.ts because types are not visible when export = is used I think.

import * as ns from 'classnames-ts';

const classNames : ns.ClassNamesFn = require('classnames');

export { classNames };

this works fine only with require in all those environments: jest, rollup, webpack

bradleyayers · 2018-02-12T20:55:03Z

@maciejw years ago when I opened this PR I used export =, as at the time this library did not have an ES module export (ES modules are not callable, so import * as classnames from "classnames"; classNames() is not valid — this is what rollup complains about with Cannot call a namespace ('classNames')). The TypeScript method of interop with callable commonjs modules is the export = declaration.

However since then the library has added support for ES modules by way of a default export (915186a). ~~So I recommend this PR be changed to use a default export rather than the callable commonjs module.~~ I've updated the PR to use a default export.

Unfortunately bind.js still is not a valid ES module (no default export), so it needs to stay as export =.

maciejw · 2018-02-12T21:17:53Z

@bradleyayers I stumbled upon similar case with not callable export = in invariant zertosh/invariant#32 library... I'm sure not last one ;) to sum it up:
commonjs 'module.export' = d.ts 'export =' witch can be imported in es6 only with import * as x from 'x' with exception to x cannot be callable, on top of this different bundlers/transpiles are adding logic, to solve this incompatibility, is this picture accurate?
whould it be a better solution to add module=index.es.js in package.json as a shim? d.ts then would be complatible with indes.es.js and index.js would be left for commonjs only

jednano · 2018-02-12T22:54:41Z

Isn't it about time this library drop support for dead/unmaintained Node.js versions? It seems only Node.js v0.10 and 0.12 are failing this build.

bradleyayers · 2018-02-13T00:30:25Z

@maciejw I've responded in zertosh/invariant#32 (comment)

dcousens · 2018-07-20T10:27:05Z

Can we not merge this to DefinitelyTyped instead?
Why should it be in this repository, the maintainers don't use TS?
Anyone willing to champion the bindings?

bradleyayers · 2018-07-20T11:16:40Z

Hi thanks for the questions. In regards to whether it can be merged to DefinitelyTyped someone will need to check the DefiniteyTyped repository to confirm it is not already there. I would be surprised if someone has not already contributed them there.

In regards to why the types should be in this repository I suspect the argument for having the types in this repository may simply boil down to https://en.m.wikipedia.org/wiki/Cohesion_(computer_science).

In regards to the maintainers not using TypeScript, I empathise with their plight and would not want them to take on any burden they do not feel comfortable with. They do amazing work for free and they should be not bullied into anything.

In regards to championing the work I believe this PR may qualify.

dcousens

LGTM.

@jedmao can you rebase your PR off this?

jednano · 2018-07-31T16:13:16Z

@dcousens I've rebased #162

ssylvia · 2018-08-21T15:12:57Z

Any idea when this will be merged or is there anything I can do to help get this merged?

I have not been able to get types working this library or by using @types/classname.

Using:

import classnames from 'classnames';

with "allowSyntheticDefaultImports": true in tsconfig.

adidahiya · 2018-08-21T15:42:37Z

@ssylvia you should try enabling both --allowSyntheticDefaultExports and --esModuleInterop

dcousens · 2018-10-04T02:40:14Z

Wait, these were copied from DefinitelyTyped...

Please add the copyright notice to the top of this file, it should be enough - though I am not a lawyer - as the LICENSE is the same.

Otherwise I'll revert.

@bradleyayers

lukescott · 2019-01-25T16:50:01Z

Is there a reason ClassDictionary is more strict that DefinitelyTyped? Real world example that doesn't work with the stricter version:

<div className={classnames("textField", error, {error})}>

error is a string that contains an error codes, such as "required". If the error is "required" then className will be "textField error required" while no error would be "textField".

With the stricter types you have to do:

<div className={classnames("textField", error, {error: !!error})}>

A bit of a nit-pick I guess, but the first one is a tad shorter and the library itself allows any as it does a truthy check.

Also it looks like the latest published version to npm does not contain the TypeScript definitions.

theKashey · 2019-10-19T22:02:58Z

Still not released.

FDiskas · 2021-03-17T08:43:58Z

Still :D
That means the project is dead?

JedWatson · 2021-04-07T22:44:46Z

@FDiskas it's released

Add TypeScript declaration.

de474aa

dcousens mentioned this pull request Dec 8, 2016

Bundle TypeScript definitions #102

Closed

dcousens assigned JedWatson Dec 8, 2016

dcousens added the feature label Dec 8, 2016

jednano mentioned this pull request Dec 10, 2016

[Question] is it possible to have separate ES modules exports #99

Closed

adidahiya suggested changes Jan 12, 2017

View reviewed changes

Update TypeScript definitions to match DefinitelyTyped, and include b…

f46d306

…ind.d.ts

jednano approved these changes Jan 17, 2017

View reviewed changes

adidahiya approved these changes Jan 17, 2017

View reviewed changes

Switch to default export

a924352

say8425 mentioned this pull request May 21, 2018

Error in typescript: Left side of comma operator is unused and has no side effects. #151

Closed

dcousens unassigned JedWatson Jul 20, 2018

dcousens mentioned this pull request Jul 20, 2018

Add tests for TypeScript import #162

Closed

dcousens approved these changes Jul 25, 2018

View reviewed changes

dcousens merged commit 2c76e87 into JedWatson:master Oct 4, 2018

dcousens mentioned this pull request Oct 4, 2018

add typescript contributor copyright notice #177

Merged

8 tasks

theKashey mentioned this pull request Oct 19, 2019

Not compatible with TypeScript #205

Closed

		@@ -0,0 +1,9 @@
		type ClassValue = string \| number \| ClassDictionary \| ClassArray;


		interface ClassArray extends Array<ClassValue> { }

		export default function(...classes: ClassValue[]): string;

Add TypeScript declaration. #103

Add TypeScript declaration. #103

Conversation

bradleyayers commented Nov 9, 2016

bradleyayers commented Nov 15, 2016

cwmacdon commented Dec 8, 2016

bradleyayers commented Dec 8, 2016

jednano commented Dec 10, 2016

bradleyayers commented Dec 11, 2016

jednano commented Dec 11, 2016

bradleyayers commented Dec 11, 2016

cwmacdon commented Jan 5, 2017

bradleyayers commented Jan 6, 2017 • edited Loading

cwmacdon commented Jan 12, 2017

adidahiya commented Jan 12, 2017

adidahiya Jan 12, 2017

Choose a reason for hiding this comment

adidahiya Jan 12, 2017

Choose a reason for hiding this comment

adidahiya Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

cwmacdon Jan 12, 2017

Choose a reason for hiding this comment

adidahiya Jan 12, 2017

Choose a reason for hiding this comment

cwmacdon Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

adidahiya Jan 12, 2017

Choose a reason for hiding this comment

cwmacdon Jan 12, 2017

Choose a reason for hiding this comment

adidahiya Jan 12, 2017

Choose a reason for hiding this comment

cwmacdon commented Jan 12, 2017

cwmacdon commented Feb 15, 2017

cwmacdon commented Feb 21, 2017

dcousens commented Feb 22, 2017

adidahiya commented Mar 2, 2017

Havret commented Apr 17, 2017 • edited Loading

adidahiya commented May 15, 2017

cwmacdon commented May 15, 2017

maciejw commented Feb 12, 2018 • edited Loading

adidahiya commented Feb 12, 2018

maciejw commented Feb 12, 2018 • edited Loading

bradleyayers commented Feb 12, 2018 • edited Loading

maciejw commented Feb 12, 2018

jednano commented Feb 12, 2018

bradleyayers commented Feb 13, 2018

dcousens commented Jul 20, 2018 • edited Loading

bradleyayers commented Jul 20, 2018

dcousens left a comment

Choose a reason for hiding this comment

jednano commented Jul 31, 2018

ssylvia commented Aug 21, 2018

adidahiya commented Aug 21, 2018

dcousens commented Oct 4, 2018 • edited Loading

lukescott commented Jan 25, 2019

theKashey commented Oct 19, 2019

FDiskas commented Mar 17, 2021

JedWatson commented Apr 7, 2021

bradleyayers commented Jan 6, 2017 •

edited

Loading

adidahiya Jan 12, 2017 •

edited

Loading

cwmacdon Jan 12, 2017 •

edited

Loading

Havret commented Apr 17, 2017 •

edited

Loading

maciejw commented Feb 12, 2018 •

edited

Loading

maciejw commented Feb 12, 2018 •

edited

Loading

bradleyayers commented Feb 12, 2018 •

edited

Loading

dcousens commented Jul 20, 2018 •

edited

Loading

dcousens commented Oct 4, 2018 •

edited

Loading