Skip to content
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

Default export not output as expected -- missing __esModule #165

Closed
rtbenfield opened this issue Jul 6, 2019 · 16 comments · Fixed by #327
Closed

Default export not output as expected -- missing __esModule #165

rtbenfield opened this issue Jul 6, 2019 · 16 comments · Fixed by #327
Labels
kind: bug Something isn't working kind: feature New feature or request

Comments

@rtbenfield
Copy link

Current Behavior

TSDX doesn't seem to have the expected output when building a package with a default export. The type definitions are output as expected, but the resulting .js file actually exports as { default: /* value */ }. I've created a minimal, reproducible example at rtbenfield/tsdx-default-export bootstrapped from tsdx create. This is my first time using TSDX and it is possible I am just doing something incorrectly.

In the repo mentioned above, I changed the sum function to be a default export, then added output.test.ts which imports directly from dist instead of src. output.test.ts logs the value of import sum from "../dist" and is showing sum to be { default: ... } instead of the actual function.

Expected behavior

The .js output files in dist can be consumed as default exports (ex. import sum from "my-module") if the source file utilized default exports.

Suggested solution(s)

Unsure, but willing to investigate if this is confirmed to be an issue. I know Webpack, but not Rollup, so it could be a good excuse for me to get some experience with it.

Additional context

Nothing relevant that I know of.

Your environment

Software Version(s)
TSDX 0.7.2
TypeScript 3.5.2
Browser N/A
npm/Yarn 6.9.0 / 1.16.0
Operating System Windows 10 Pro 1903
@swyxio
Copy link
Collaborator

swyxio commented Jul 6, 2019

confirmed that i have run into this issue before, but jared has a strong view that we shouldnt use default exports anyway and this is his tool so i didnt bother doing anything about it. if you'd like to introduce a warning, or error, or fix the output as long as it doesnt affect the experience for named exports, i'd be open to that

@agilgur5
Copy link
Collaborator

agilgur5 commented Jul 16, 2019

Just experienced this bug too in my package (mst-persist, referenced above). This caused an unintentional breaking change as the lack of __esModule in the CJS output means default imports don't work in transpiled code.

Per @rtbenfield's example, if one is transpiling their code from ESM to CJS, they might be using default imports which will then fail when transpiled.
Most testing tools run in Node and don't have out-of-the-box support for ES Modules without transpilation (including Jest - one needs to configure the esm package to work without transpiling)

This bug is caused by the Rollup config here:

// Do not let Rollup add a `__esModule: true` property when generating exports for non-ESM formats.
esModule: false,

I think this option should be changed to respect the tsconfig setting of esModuleInterop. The current 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 problem with changing that code is that it doesn't import tsconfig.json -- opts.tsconfig is a string, so one can't read tsconfig.esModuleInterop without importing it somewhere. A quick hack would be to dynamically import/require it right there though.

as long as it doesnt affect the experience for named exports, i'd be open to that

Changing this option so that __esModule is in the output should not affect named exports, it's just a hint for transpilers to use .default when transpiling default imports to CJS

@jaredpalmer
Copy link
Owner

Ahh. Yeah I'm sorry about this guys. I don't use default exports and I can go in depth as why you should not either. We should probably just document this in the README if there is no quick fix

@agilgur5
Copy link
Collaborator

@jaredpalmer the quick fix would be:

esModule: require(opts.tsconfig).esModuleInterop,

I can submit a PR for that, but it's pretty hacky so idk if that's acceptable.

I think the big issue with not supporting default exports is that it limits tsdx to use only by 1) new libraries, 2) libraries that already don't use default exports, or 3) libraries willing to make a breaking change to incorporate tsdx.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jul 25, 2019

Upon testing, the quick fix does not seem to work as opts.tsconfig is only defined when you pass a custom tsconfig to tsdx. Not entirely sure where a standard app root tsconfig is read (it's not in tsdx source code afaik, so I guess only in rollup-plugin-typescript2?).

Could make that the default arg for the option.
Alternatively, could check against the app root right here as well if opts.tsconfig is undefined, but that gets even more hacky 😕

The slightly less hacky way to "import" app root tsconfig or opts.tsconfig would be to do it the same way app package.json does it I guess:

tsdx/src/index.ts

Lines 43 to 50 in 1a2e50c

let appPackageJson: {
name: string;
source?: string;
jest?: any;
};
try {
appPackageJson = fs.readJSONSync(resolveApp('package.json'));
} catch (e) {}

@benlesh
Copy link

benlesh commented Oct 16, 2019

I was just burned by this as well. I know that's not helpful feedback, but I wanted to ping this issue again so it gets attention. This is sort of an Achilles heel to this tool at the moment.

@jaredpalmer
Copy link
Owner

Does it not work with 10.x?

@benlesh
Copy link

benlesh commented Oct 16, 2019

I started a new project just today, fresh install of tsdx, everything seemed okay, but when I tested out what I published it was off. Typings pointed to default, actual value was at default.default.

Here's the repo: https://github.com/benlesh/react-simple-vlist

I've since flipped it to not use default exports for 0.0.1, but you can se the problem with 0.0.0.

@kunukn
Copy link

kunukn commented Oct 29, 2019

Have you tried?

module.exports = function myThing() {}

Instead of export default function myThing() {}

This worked for me.

Example project.
https://github.com/kunukn/use-mount

@TrySound
Copy link
Collaborator

Or just don't use this devil syntax. Named exports do the work better.

@nartc
Copy link

nartc commented Nov 12, 2019

Ran into this issue today. It's not that I (and some others) want to use Default Export but because some other tools require Default Export to work, namely Mocha Custom Reporter.

@agilgur5
Copy link
Collaborator

Added a PR to resolve this in #327 following my previous suggestions above.

As I and others have stated above, this is a pretty big blocker to adoption and causes lots of unexpected compatibility bugs with different tools, as well as unintended breaking changes for any libraries that try to adopt tsdx without knowing about this bug / caveat. "just don't use default exports" is an oversimplification and not a valid solution. I actually specifically haven't used tsdx in other libraries I maintain because of this issue and its various consequences.

#327 respects the esModuleInterop setting in tsconfig, such that if you're using CJS and default exports in any way (whether as an option for your users, because your tooling requires it, etc, etc), you can enable that and tsdx will properly output __esModule in CJS builds.
As mentioned in my investigations above, export default and exports.default are already output if you used default exports, this PR solely adds the option to output __esModule.

@jaredpalmer
Copy link
Owner

@agilgur5 as you know, tsdx is just formik's build system. I don't use default exports unless forced to by a framework (e.g. next.js or gatsby). I apologize for the frustration. Is there any workaround?

@nartc
Copy link

nartc commented Nov 14, 2019

Have you tried?

module.exports = function myThing() {}

Instead of export default function myThing() {}

This worked for me.

Example project.
https://github.com/kunukn/use-mount

@jaredpalmer ^ this workaround worked for me. https://github.com/ArchitectNow/cypress-testrail-reporter/blob/master/src/reporter.ts#L61

nickbreaton added a commit to nickbreaton/styled-when that referenced this issue Nov 18, 2019
@agilgur5
Copy link
Collaborator

agilgur5 commented Nov 20, 2019

@nartc the workaround that @kunukn listed isn't entirely compatible.

It'll make const myThing = require('myThing') work for CJS users, but now the ESM build won't work. You can add module.exports = myThing in addition to export default myThing and then it seems to work for ESM (but I have no idea how consistent that is as it's mixing CJS syntax with ESM syntax).

BUT if you use module.exports = myThing, then CJS users can only use this one export. You can't add keyed exports for CJS users that way -- CJS doesn't have named exports, it just exports one thing. exports.default is how transpilers and bundlers have resolved that, but that means you have to do const { default: myThing } = require('myThing') in CJS. CJS and ESM are fundamentally incompatible; this is one of the reasons why Node has had trouble adopting it for years (not to mention static vs dynamic, sync/async, etc, etc -- can read a brief, somewhat out-of-date summary on that topic here).
Note that most users will probably be CJS users as most tooling does in fact transpile things down, particularly because Node only supports ESM in newer versions, and at that, only behind the --experimental-modules flag.

The __esModule property is what tells a transpiler that:

import myThing from 'myThing'

should transpile to

const { default: myThing } = require('myThing')

and not

const myThing = require('myThing')

As I've said in my above comments, the lack of __esModule is the root cause of this problem. And my fix in #327 resolves that.

@agilgur5
Copy link
Collaborator

I realized earlier today that there is a workaround for this in tsdx.config.js (added in the Aug 27th v0.9 release):

module.exports = {
  rollup(config, options) {
    config.output.esModule = true
    return config
  }
}

My fix got merged today and should be out whenever the next release is cut, but in the meantime or for slightly older versions of tsdx, could use that workaround.

@agilgur5 agilgur5 changed the title Default export not output as expected Default export not output as expected -- missing __esModule Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working kind: feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants