-
Notifications
You must be signed in to change notification settings - Fork 118
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
feat!: adds support for ESM and Deno #295
Conversation
// Cleanup the export statement in CJS typings file generated by rollup: | ||
const legacyTypings = 'build/index.cjs.d.ts' | ||
const contents = readFileSync(legacyTypings, 'utf8').replace( | ||
'export { yargsParser as default };', 'export = yargsParser;' |
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.
Is this a bug in rollup's support for --exports=default
? If so, is there an open issue this could link to track when the workaround may become unnecessary?
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.
@jkrems I wanted to keep the behavior of the CJS module identical to what it is today, with the main export being a callable function, i.e., not exports.default = yargsParser
. However, for ESM/Deno I wanted to export default yargsParser
... I wasn't sure how to indicate to rollup that I wanted the export = yargsParser
behavior, while at the same time using the desired syntax in the source files themselves.
Maybe I should open a ticket on the tsc rollup plugin I'm using?
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.
Yeah, it seem like if the module.exports
object is set to the default export, then export = defaultExport
would be the correct typings to emit. At least from what I understand about TS type definitions. Not sure how that would affect future ESM consumers (that use vscode code completion) in the presence of named exports etc. but that's not necessarily a problem for today..?
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.
Exciting!
Co-authored-by: Daniel Stockman <5605+evocateur@users.noreply.github.com>
Co-authored-by: Daniel Stockman <5605+evocateur@users.noreply.github.com>
Using @mleguen and @QmarkC's work as a starting point, this refactor adds support for ESM and Deno.
The approach taken is as follows:
index.cjs.d.ts
(which is the binding that will be used forrequire
statements).Todo:
tokenize-arg-string.ts
.BREAKING CHANGE:
decamelize
andcamelCase
logic has been re-implemented, and may have some logical differences. Conditional exports for CJS vs., ESM may break some environments.