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

Generate CJS version of otpauth.d.ts file #447

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Generate CJS version of otpauth.d.ts file #447

merged 5 commits into from
Dec 6, 2023

Conversation

wpf500
Copy link
Contributor

@wpf500 wpf500 commented Nov 30, 2023

I use your library in a TypeScript project and have just been updating my build. When I changed my TSConfig option module from commonjs to node16 I started getting the following error when importing your library:

import { TOTP } from "otpauth";
// The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("otpauth")' call instead.
//  To convert this file to an ECMAScript module, change its file extension to '.mts' or create a local package.json file

After a lot of digging it turns out this is because while your library does specify an export for Node CommonJS libraries (
https://github.com/hectorm/otpauth/blob/master/package.json#L44), it doesn't specify a CommonJS type definition file. TypeScript assumes that because the type definition file ends d.ts rather than d.cts the package is an ESM package and fails (more info here: microsoft/TypeScript#53045)

This PR adds the relevant lines that tells TypeScript that it can treat this as a CommonJS library. Note moving types down in exports is necessary because otherwise it matches the first types before getting to the Node CommonJS specific one.

Copy link
Owner

@hectorm hectorm left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I don't usually work with TypeScript, but I've reviewed your changes and although they look good, I think they could be improved with the suggested changes.

package.json Outdated Show resolved Hide resolved
rollup.config.mjs Outdated Show resolved Hide resolved
rollup.config.mjs Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@wpf500
Copy link
Contributor Author

wpf500 commented Dec 6, 2023

@hectorm I think I've addressed all your comments now

@hectorm hectorm merged commit 352e6cc into hectorm:master Dec 6, 2023
14 checks passed
@hectorm
Copy link
Owner

hectorm commented Dec 6, 2023

LGTM, thanks!

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.

2 participants