Open karlhorky opened 1 year ago
cc @andrewbranch in case you can see an error in my logic about "module": "node16"
+ PR above, would be amazing to have a tip here!
slugify’s JS uses a UMD wrapper that includes
module.exports = factory()
module.exports['default'] = factory()
so technically the export default
in the types isn’t wrong, it’s just incomplete. In your repro, you can do slugify.default('')
and TypeScript will recognize that as good, and it will work in Node too. But obviously you want to be able to get at the module exports without the unnecessary .default
. To do that, off the top of my head, I would write the types like this:
declare module slugify {
type ExtendArgs = {
[key: string]: any;
}
export function extend (args: ExtendArgs): void;
const _default: typeof slugify;
export { _default as default };
}
declare function slugify(
string: string,
options?:
| {
replacement?: string;
remove?: RegExp;
lower?: boolean;
strict?: boolean;
locale?: string;
trim?: boolean;
}
| string,
): string;
export = slugify;
export as namespace slugify;
Note the const _default: typeof slugify; export { _default as default };
in the existing merged namespace as well as the export = slugify
change. Together, those cover both the module.exports
as well as the module.exports.default
. The export as namespace slugify
just declares that the module is also available as a global in script files, since it does have a browser-compatible UMD wrapper. That can be dropped if global usage is discouraged.
Also, I will note that this kind of problem is not yet detectable by https://arethetypeswrong.github.io, but it’s next on my list.
Ok great, added the changes for the UMD wrapper in b443c79b89ec6700513afcb70973258d438ef76a . I checked and this seems to also work in my project with "module": "node16"
@simov @Trott what do you think?
Edit: I realized that the issue I described in my comment was not caused by the changes proposed in this PR. Actually, merging this PR will be solving my problem
Is there a reason to use module
over namespace
here? Also is there any particular reason to define exports in the module instead of an interface?
I think it’s purely stylistic, but I’m curious to know if there are any differences. I would write it like this:
declare namespace slugify {
type ExtendArgs = {
[key: string]: any;
}
interface Options {
replacement?: string;
remove?: RegExp;
lower?: boolean;
strict?: boolean;
locale?: string;
trim?: boolean;
}
}
interface Slugify {
(string: string, options?: slugify.Options | string): string;
default: Slugify;
extend: (args: slugify.ExtendArgs) => void;
}
declare const slugify: Slugify;
export = slugify;
export as namespace slugify;
There’s no difference, and namespace
is the preferred keyword. I used module
only because I copied and pasted the original code.
Hi there 👋 First of all, thanks for
slugify
, it's very useful!The ESM-style export (
export default slugify
) causes an error when using the"module": "node16"
option intsconfig.json
and"type": "module"
inpackage.json
:Demo on StackBlitz - run
yarn tsc
in the Terminal at the bottom to reproduce:https://stackblitz.com/edit/node-wshdsj?file=tsconfig.json,package.json,index.ts&file=tsconfig.json,index.ts
Background
The change to ESM-style export in https://github.com/simov/slugify/pull/19 seemed like the right solution at the time, because bundlers such as webpack +
ts-loader
did not understand the CommonJS syntax (export = slugify
).However, module systems have evolved since then and now the
export default
fails when"module": "node16"
is configured intsconfig.json
inside of a project which is an ES Module ("type": "module"
inpackage.json
).Since
slugify
is a CommonJS module, it seems like using a CommonJS-style export is indeed the right choice for this package.See more information about the problem of misconfigured declaration files in packages here: