rollup / rollup-plugin-node-resolve

This module has moved and is now available at @rollup/plugin-node-resolve / https://github.com/rollup/plugins/blob/master/packages/node-resolve
MIT License
468 stars 96 forks source link

The generated CSJ and ES files in this project do not match, causing the TypeScript definition to not work for CSJ. #236

Closed MicahZoltu closed 5 years ago

MicahZoltu commented 5 years ago

How Do We Reproduce?

Try to use this plugin from TypeScript.

import nodeResolve from 'rollup-plugin-node-resolve'
nodeResolve()

Expected Behavior

It compiles and runs.

Actual Behavior

It compiles, but the generated code looks like:

const rollup_plugin_node_resolve_1 = require("rollup-plugin-node-resolve");
rollup_plugin_node_resolve_1.default()

This is because module.exports = myFunction is NOT the same as module.exports.default = myFunction. A quick glance over this code suggests that you are using rollup to generate the cjs and es dist files, so I guess the bug here is that rollup is generating code CJS code that is incompatible with the ES code it generates in terms of resulting shape, which means a single TypeScript definition file cannot be used for both.

The ideal solution to this is to not use default exports and instead use named exports. This way you can do plugin.nodeResolve in either ES or CJS. A more backward compatible solution would be to create two different .d.ts files that sit beside the JS files. One would be named rollup-plugin-node-resolve.cjs.d.ts and the other named rollup-plugin-node-resolve.es.d.ts. These would look similar except have different shape for the exported function. To avoid code duplication you could create a single shared .d.ts file that has all of the definitions, then do export * from 'shared' and define the "default" export individually in each.

lukastaegert commented 5 years ago

Did you add the --allowSyntheticDefaultImports compiler flag? I think TypeScripts assumption that the default import is somewhere on a default property is understandable but will not work for most the the remaining Node ecosystem either.

MicahZoltu commented 5 years ago

According to TypeScript documentation:

Allow default imports from modules with no default export. This does not affect code emit, just typechecking.

At the moment, type checking is working, it is runtime that is the problem. For the current setup to work, I believe TypeScript would need a way to emit non-default require statements when emitting for commonjs targets.

(also, I did test with that change to my project and it had no effect)

MicahZoltu commented 5 years ago

tsconfig.json compiler option:

"esModuleInterop": true,

seems to have done the trick.

lukastaegert commented 5 years ago

"esModuleInterop": true

Ah yes, I keep mixing up the two, thanks for checking.