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

fix: fix esm support and types #57

Merged
merged 1 commit into from
Jul 15, 2023
Merged

fix: fix esm support and types #57

merged 1 commit into from
Jul 15, 2023

Conversation

remcohaszing
Copy link
Contributor

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]
}

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.
@lukeed
Copy link
Owner

lukeed commented Nov 1, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

lukeed commented Nov 1, 2022

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
Copy link

wojtekmaj commented Jul 12, 2023

@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
Copy link
Owner

lukeed commented Jul 12, 2023

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

@lukeed lukeed reopened this Jul 15, 2023
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

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

❗ 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
@@            Coverage Diff            @@
##            master       #57   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           22        22           
=========================================
  Hits            22        22           

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

@remcohaszing
Copy link
Contributor Author

Thanks for merging and releasing this! ❤️

@remcohaszing remcohaszing deleted the fix-esm-support branch July 16, 2023 06:55
@lukeed
Copy link
Owner

lukeed commented Jul 16, 2023

Thank you for the PR & patience!

carlblock added a commit to carlblock/react-draggable that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants