jaredpalmer / tsdx

Zero-config CLI for TypeScript package development
https://tsdx.io
MIT License
11.28k stars 507 forks source link

UMD format with lodash breaks due to lodash-es substitution #759

Open zhaoyao91 opened 4 years ago

zhaoyao91 commented 4 years ago

Current Behavior

To export umd module, I would like to include all modules into the bundle, so I config the rollup

    if (config.output.format === 'umd') {
      delete config.external;
    }

but when building, it crashes:

(babel plugin) SyntaxError: /Users/bytedance/workspace/ee_web_apps-menu/node_modules/lodash-es/isBuffer.js: 'import' and 'export' may only appear at the top level (4:0)

  2 |
  3 | var isBuffer_1 = commonjsHelpers.createCommonjsModule(function (module, exports) {
> 4 | import root from './_root.js';
    | ^
  5 | import stubFalse from './stubFalse.js';
  6 |
  7 | /** Detect free variable `exports`. */

at undefined:4:0

Expected behavior

Suggested solution(s)

when building umd module, it should not rename lodash to lodash-es

Additional context

Your environment

Software Version(s)
TSDX ^0.13.2
TypeScript
Browser
npm/Yarn
Node
Operating System
agilgur5 commented 4 years ago

when building umd module, it should not rename lodash to lodash-es

Ah yep, that's probably required for UMD. Currently the code only checks for CJS. Should be a one-liner fix here: https://github.com/formium/tsdx/blob/8b148cea34c1852f6fcf6e94a3758d7a9f46d9b2/src/babelPluginTsdx.ts#L72

EDIT: actually I'm not so sure this isn't due to an upstream bug. ~There's a CJS plugin for Rollup used that should convert lodash-es too.~ (Misinterpreted that plugin, it does the inverse) The error is coming from Babel and not Rollup, and it's complaining about location of import and not usage of import 🤔 Though I think the one-liner fix would still resolve this error.

Morphexe commented 4 years ago

How do I disable this feature ?

agilgur5 commented 4 years ago

@Morphexe not sure what you mean, this is a bug, not a feature.

I think one can do a partial workaround for this by configuring babelrc with a dummy entry:

module.exports = {
  // ...
  plugins: [
    // ...
    ['babel-plugin-transform-rename-import', { replacements: [{ original: '', replacement: '' }] }]
  ]
}

But conversely, that'll probably break ESM builds

agilgur5 commented 4 years ago

So https://github.com/formium/tsdx/issues/912#issuecomment-717002753 revealed a similar issue that points to upstream https://github.com/rollup/plugins/issues/304 which would seem to confirm my suspicion that there's a bug upstream.

Per my comment there:

That seems to be fixed in v12.0.0, but that version has an undocumented requirement on Rollup 2, which is a very breaking change slated for v0.15.0.

Will have to add a test for this in v0.15.0 to ensure it works after the upgrade.

axedre commented 3 years ago

@Morphexe not sure what you mean, this is a bug, not a feature.

I think one can do a partial workaround for this by configuring babelrc with a dummy entry:

  // ...
  plugins: [
    // ...
    ['babel-plugin-transform-rename-import', replacements: [{ original: '', replacement: '' }]
  ]
}

But conversely, that'll probably break ESM builds

Can you post a working example please? The snippet syntax looks borked...

agilgur5 commented 3 years ago

@axedre ah looks like it was missing a closing bracket, I updated my comment

axedre commented 3 years ago

That still doesn't look like valid javascript. For anyone else stumbling on this, check out: https://www.npmjs.com/package/babel-plugin-transform-rename-import.