Fix TypeScript types when using require

[remcohaszing]

The package uses commonjs, but it’s typed as if it contains faux ESM. This was problematic for people using require or the new node16 module resolution.

[dzearing]

Please @JedWatson could you help merge/publish this? This is blocking cases where TypeScript "nodenext" resolution is being used. See this issue for details: microsoft/TypeScript#49189

[adidahiya]

Hi, FYI this is a breaking change for TypeScript consumers which is unexpected in a patch version of the classnames package.

Before, we could import classNames from "classnames" without enabling esModuleInterop in TS compilation options. Now, that import syntax is no longer valid unless we enable esModuleInterop: true.

I understand that this change was made to support a newer TS resolution option, but changes to typings like this should be done carefully to avoid breaking existing consumers.

May I ask why you are using "moduleResolution": "nodenext"? Are you running classnames in Node.js / isomorphic rendering? IMO that's a less common use case than using classnames in a frontend-only bundle, where node resolution is not used/necessary. So this "fix" may have broken usage for a larger group of consumers than it actually fixed anything for.

[remcohaszing]

I see how this affects you and why you consider this breaking. However, I believe this was a bug fix. Any change of behaviour can break code, including bug fixes. You were previously relying on this bug.

According to the readme, correct usage of this package is:

// CJS
const classNames = require('classnames')

// ESM
import classNames from 'classnames'

However, before this PR, according to the TypeScript type definitions, correct usage was:

// CJS
const classNamesExports = require('classnames')
const classNames = classNamesModule.default

// ESM
import classNamesExports from 'classnames'
const classNames = classNamesModule.default

The latter technically works too, because classnames defines module.exports.default. The default property was added in 915186a, but no context is given. My guess is this was to please people who didn’t configure TypeScript properly.

The latter didn’t work because it was typed correctly. It worked by accident. The situation you are describing is usage of this package inside a module bundler that allows you to import module.exports as default. This means you don’t rely on the property module.exports.default. To support such situations, TypeScript has the option allowSyntheticDefaultImports. Note that you don’t need esModuleInterop, because you’re not compiling to CJS.

May I ask why you are using "moduleResolution": "nodenext"? Are you running classnames in Node.js / isomorphic rendering?

The JavaScript ecosystem is slowly moving towards ESM. In order to be able to use newer packages or updates, one has to keep up. "moduleResolution": "node16" adds support for various cases that weren’t previously supported. It it also more strict, and highlights bugs like the one fixed by this PR.

IMO that's a less common use case than using classnames in a frontend-only bundle, where node resolution is not used/necessary.

This isn’t a matter of opinion, but of statistics that aren’t presented. I’m inclined to believe this is correct though. What matters is correctness though, not usage statistics.

So this "fix" may have broken usage for a larger group of consumers than it actually fixed anything for.

This didn’t break usage. It highlights a problem in those consumers’ configuration. You would’ve run into the same problem eventually if you added another properly typed dependency.

For more information, see https://github.com/DefinitelyTyped/DefinitelyTyped#a-package-uses-export--but-i-prefer-to-use-default-imports-can-i-change-export--to-export-default

[remcohaszing]

I stand by my previous comment, but I learnt that it is possible to also support the .default property, allowing default imports to work without allowSyntheticDefaultImports. This is accurate only because classNames specifies the .default property. For typical CJS libraries this should not be applied. Also support for UMD was missing in this PR. Both are added in #301.