-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Corrected type definitions. Fixes #56 #57
Conversation
This PR corrects the TypeScript definitions to properly reflect the fact that Unified exports a CommonJS module rather than an ESM module. * Replaced incorrect `export default` syntax with `export =` * Wrapped all the TypeScript type definitions in a namespace
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.
Can you add a test case to types/unified-tests.ts
to verify the commonjs style import works as expected?
As it stands now, the tests passed before and continue to pass, and building the code worked in wbepack before and works with this change.
Without a test case, there is not way prevent this issue from popping up again.
@@ -6,7 +6,6 @@ | |||
"noImplicitThis": true, | |||
"strictNullChecks": true, | |||
"strictFunctionTypes": true, | |||
"esModuleInterop": true, |
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.
Would documenting that esModuleInterop
should be enabled, resolve this issue without reverting the typings refactor?
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.
The esModuleInterop
setting should only be used in end applications, not libraries. It's a way for app developers to treat CommonJS modules as though they were ESM modules, but it results in lots of little helper functions being injected into the code to handle the mapping between the two module formats. App developers should be able to opt-in or opt-out of that behavior, depending on whether they're okay with the trade-off.
/cc @Rokt33r |
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.
Makes sense to me. Thanks @JamesMessinger !
How should this be released? |
@wooorm Did you mean the version? If so, we could publish as a patch version, like |
Yeah, I meant version. So a patch is fine? Alright! |
Related to unifiedjs/unified#53. Related to unifiedjs/unified#54. Related to unifiedjs/unified#56. Related to unifiedjs/unified#57. Related to unifiedjs/unified#58. Related to unifiedjs/unified#59. Related to unifiedjs/unified#60. Related to unifiedjs/unified#61. Related to unifiedjs/unified#62. Related to unifiedjs/unified#63. Related to unifiedjs/unified#64. Related to #426. Reviewed-by: Titus Wormer <tituswormer@gmail.com> Reviewed-by: Junyoung Choi <fluke8259@gmail.com> Reviewed-by: Christian Murphy <christian.murphy.42@gmail.com> Co-authored-by: Junyoung Choi <fluke8259@gmail.com> Co-authored-by: Christian Murphy <christian.murphy.42@gmail.com>
This PR fixes #56 by correcting the TypeScript definitions to properly reflect the fact that Unified exports a CommonJS module rather than an ESM module.
export default
syntax withexport =