-
Notifications
You must be signed in to change notification settings - Fork 508
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
(fix): respect tsconfig esModuleInterop flag #327
Conversation
- the previous assumption that ESM users will always import the ESM build doesn't actually hold up in practice due to various limitations in Node and testing and the prevalence of transpiling in general - even if one uses mostly ESM, many tools will require and use the CJS build, and this option would break compatibility - without esModule set, CJS users would be unable to use the default export of any library built with tsdx - and this was not documented, meaning it would cause unintended breaking changes in libraries that use default exports and try to adopt tsdx - it would also break compatibility with certain tooling - so, instead of always setting rollup's esModule to false and potentially causing unintended consequences, respect the user's esModuleInterop config as set in their tsconfig, but default to false
13bd5ab
to
838e447
Compare
@benlesh does this solve your issue? |
@jaredpalmer it's been almost a month since I made this PR to fix the well-upvoted default export bug (#165), what can we do to get this merged? I'm currently running a fork of |
* master: (26 commits) (deps/lint): upgrade @typescript-eslint to support ?. and ?? (jaredpalmer#377) (ci): add a lint job so PRs will require passing lint (jaredpalmer#378) (clean): remove .rts_cache_* from storybook gitignore (jaredpalmer#375) Add optional chaining and nullish coalescing operators support (jaredpalmer#370) Added Storybook template (jaredpalmer#318) (fix/ci): GitHub Actions should run on PRs as well (fix/format): formatting of jaredpalmer#366 didn't pass lint Add prepare script to generated project (jaredpalmer#334) default jest to watch mode when not in CI (jaredpalmer#366) (fix): respect tsconfig esModuleInterop flag (jaredpalmer#327) fix: minor typo update rollup deps and plugins update to ts 3.7 Remove unnecessary yarn install command in GH action update README.md update README.md Use node_modules/.cache/... as cacheRoot (jaredpalmer#329) fix(lint): Only default to src test if they exist (jaredpalmer#344) Fix error when providing babel/preset-env without options (jaredpalmer#350) Replaced some sync methods for their async version ...
the previous assumption that ESM users will always import the ESM
build doesn't actually hold up in practice due to various limitations
in Node and testing and the prevalence of transpiling in general
the CJS build, and this option would break compatibility
without esModule set, CJS users would be unable to use the default
export of any library built with tsdx
breaking changes in libraries that use default exports and try to
adopt tsdx
so, instead of always setting rollup's esModule to false and
potentially causing unintended consequences, respect the user's
esModuleInterop config as set in their tsconfig, but default to
false
Fixes #165 according to my suggestion in #165 (comment) . Mine and other folk's comments in that issue give more details about the compatibility and adoption issues that drive this PR. Ultimately, I believe this is a bugfix as the current behavior causes unintended consequences and because default exports are already output, just not with
__esModule
in CJS builds.Have tested this successfully locally. All it does is, if
esModuleInterop
is set totrue
intsconfig.json
, will add a line of code to the CJS output as such:As I mentioned in #165 (comment), everything else is the same as before, as ESM builds already output
export default
and CJS builds already outputexports.default
.__esModule
is just a hint for transpilers to use.default
when transpiling default imports to CJS.