-
Notifications
You must be signed in to change notification settings - Fork 265
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
Add TypeScript and ESM support #163
Conversation
}, | ||
"rules": { | ||
"tsdoc/syntax": "warn", | ||
// Aligns with our style-guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the actual style-guide here instead of copy/pasta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still doing this @mrmckeb?
switch (compilerOptions.module) { | ||
case ts.ModuleKind.CommonJS: { | ||
// Adds backwards-compatibilty for Node.js. | ||
contents += `module.exports = exports.default;\nmodule.exports.default = exports.default;\n`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this earlier, @MatthiasKunnen!
Right now TypeScript doesn't really support "mixed outputs". Although, it looks like it will be fixed/improved in TypeScript 4.5 from what I can see.
- Support for NodeJS 12.7+ package exports microsoft/TypeScript#33079
- Initial support for module: node12 microsoft/TypeScript#44501
The way this is now built, the package (in a CJS environment) would need to be imported with require('ms').default
. That's fine, but previously you could import ms
as require('ms')
- so this change ensures backwards compatibility for those users.
As we can only really have one types file for the root import, this still means that a TS user using require
would still be asked to change it to require('ms').default
... but this at least ensures compatibility for JS-users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, we could change the CJS export to be require('ms/cjs')
(as we'd want ESM to be our default), but again that would make this a bigger breaking change for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but my approval likely doesn't mean much here 😄
Are we not running tests on each PR? |
TODO: - Requires Typescript 4.5 for package exports microsoft/TypeScript#33079, vercel/ms#163
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Let's ship this as canary dist-tag
first so we can try it out 👍
This PR:
Resolves #159.
Todo:
export =
instead ofexport default
.module.exports
alongsideexports.default
.