lukeed / clsx

A tiny (239B) utility for constructing `className` strings conditionally.
MIT License
8.08k stars 141 forks source link

fix: fix esm support and types #57

Closed remcohaszing closed 1 year ago

remcohaszing commented 1 year ago

The non-standard package.json field module was used. This pointed to a faux ESM file (Uses ESM syntax, but has a .js file extension in a package which doesn’t specify "type": "module".

ESM support was fixed by using the .mjs file extension for the ESM export and defining a proper exports field in `package.json.

The types reflected a package that has the module.exports.default field. This was incorrect. The CommonJS types have now been fixed to use export =, which is the correct way to type modules that use module.exports assignments.

Additional types were added for ESM support.

This fixes the following issues that have previously been closed without a solution.

Closes #27 Closes #43 Closes #50

To try this locally, try running the following in the Node.js repl from the package root:

$ node
Welcome to Node.js v18.12.0.
Type ".help" for more information.
> require('clsx')
<ref *1> [Function: r] { clsx: [Circular *1] }
> await import('clsx')
[Module: null prototype] {
  clsx: [Function: clsx],
  default: [Function: clsx]
}
lukeed commented 1 year ago

clsx already works with require and import and import() statements:

Screen Shot 2022-11-01 at 10 50 44 AM

The rest of this PR is adding breaking changes:

  1. Adding exports map (always is), and is unnecessary since there's only 1 file/entry point
  2. The module change to a .mjs extension breaks most older bundler versions (rollup/webpack) since it changes how they will handle module resolution for the rest of the tree. (Dark, early ESM days)
  3. Using .d.mts is strictly unnecessary & binds you to a minimum TS version for no reason at all, especially given the export = clsx contents.
remcohaszing commented 1 year ago

clsx already works with require and import and import() statements:

Screen Shot 2022-11-01 at 10 50 44 AM

Yes, commonjs files can be imported from ESM. The fact that there’s a faux ESM file and TypeScript type definitions describe a default export instead of export =, gave the impression the intended behaviour was to support actual ESM. A lot of packages have there type definitions wrong, which has become more apparent since the release of TypeScript 4.7.

The rest of this PR is adding breaking changes:

  1. Adding exports map (always is), and is unnecessary since there's only 1 file/entry point

Whether or not this is breaking is subjective. This disallows people to import subpaths from the package. I would consider that unintemded behaviour and therefor not consider this to be a breaking change. However, since you’re the maintainer, your view on this counts.

Adding an exports map isn’t unnecessary when adding native ESM support. However, if you’re not interested, then indeed the exports map is unnecessary.

  1. The module change to a .mjs extension breaks most older bundler versions (rollup/webpack) since it changes how they will handle module resolution for the rest of the tree. (Dark, early ESM days)

I wasn’t aware of this. It is possible to support both though (but not without exports).

  1. Using .d.mts is strictly unnecessary & binds you to a minimum TS version for no reason at all, especially given the export = clsx contents.

This isn’t true. This file is only used by the node16 / nodenext module resolution to support native ESM. The index.d.ts is still used by other module resolutions as it always was.

lukeed commented 1 year ago

Whether or not this is breaking is subjective.

Node 13-13.7 had a different exports map specification. The finalized spec throws an error in these versions and there’s no way to support both. Thus adding sny exports map is breaking as it has to be done with a minimum version bump.

This isn’t true.

Yes it is. You can define “types” for exports to a standard .d.ts file, even for nodenext/node16 mode. The mts extension is just to force/guarantee that TS treats the file as ESM, similar to .mjs even tho .js can also contain ESM. In fact, mts is a front/stand-in for would-be-emitted .mjs files.

wojtekmaj commented 1 year ago

@lukeed I kindly ask you to reconsider this or equivalent PR. Incorrect types are causing us a ton of problems which may force us to replace clsx with correctly typed classnames in many repositories and that would be a huge bummer.

lukeed commented 1 year ago

I'll cut a new major version that includes an "exports" map

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (6da37d6) 100.00% compared to head (40a074e) 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #57 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 22 22 ========================================= Hits 22 22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

remcohaszing commented 1 year ago

Thanks for merging and releasing this! ❤️

lukeed commented 1 year ago

Thank you for the PR & patience!