Tsdx: Esmodule options in rollup output is always undefined

[etienne-dldc]

Hi,

While working on #468 I noticed this line of code

esModule: tsconfigJSON ? tsconfigJSON.esModuleInterop : false,

But esModuleInterop is a compiler option so I guess it should be

esModule: Boolean(tsconfigJSON?.compilerOptions?.esModuleInterop)

I can create a PR to fix this I just want to be sure that I'm not missing something here...

[agilgur5]

Oh sh*t, you're right, that's actually my code from #327 😨 . We don't have automated tests for tsconfig properties (we should! for #468 too preferably), but I actually have a fork of TSDX I used specifically for this feature and it worked there.... 😖 🤔 🤔

Let me take a look at it when I get the chance. This syntax is preferred though.

I'm also working on adding tests for custom babel config (c.f. #443 ); custom tsconfig is actually easier (no new dependencies).

[agilgur5]

Ok, dug into this and found some strange results. The library I initially wrote this fix/feature for does indeed have Object.defineProperty(exports, '__esModule', { value: true }); in its dist/ output, see https://unpkg.com/[email protected]/dist/mst-persist.cjs.development.js. So does https://unpkg.com/[email protected]/dist/jest-without-globals.cjs.development.js. Both have tsconfig.compilerOptions.esModuleInterop set to true.

So did some further sleuthing and some testing and turns out esModuleInterop is actually almost always _true_ now 😅 😅. To be specific, tsconfigJSON ? tsconfigJSON.esModuleInterop : false always evaluates to undefined -- and rollup has output.esModule set to true by default. It'll only be set to false if there is no tsconfig.json.

So yea, a PR would fix this. I think a test should re-use the directory in #468 (which might take a bit to get merged as Jared will likely be the one to merge it), and then could do something like:

const buildExports = require('../stage-build/dist/index.js');
expect(buildExports.__esModule).toBe(true);

[etienne-dldc]

OK, I'll wait for #468 to be merged then

[agilgur5]

👍 Do you think you can rename this issue in the meantime? Just to avoid any confusion

[agilgur5]

@etienne-dldc now that #468 is merged would you like to PR the fix to this? I can merge that in a lot faster myself now that I'm a collaborator

[etienne-dldc]

Sorry for the late response...

Thanks for taking care of this 😃

[agilgur5]

No need to apologize, 2 days to respond is pretty normal. I just wanted to get all the tsconfig changes & tests done in one go