developit / microbundle

📦 Zero-configuration bundler for tiny modules.
https://npm.im/microbundle
MIT License
8.07k stars 363 forks source link

[Bug] Wrong function scope with rest parameters after transpiled #1037

Open zhanghaobin opened 1 year ago

zhanghaobin commented 1 year ago

description

async function with rest parameters seems got wrong function scope after transpiled

version

microbundle@^0.15.1

reproduce demo

// tsconfig.json

{
  "compilerOptions": {
    "esModuleInterop": true,
    "target": "ES6",
    "strict": true,
    "moduleResolution": "node",
    "experimentalDecorators": true,
    "baseUrl": ".",
    "allowJs": true
  },
  "include": ["*.ts"]
}
// test.ts

export const fn = async (...args: unknown[]) => {
  await 1;
  [].forEach(() => console.log(...args))
}
// test.js (generated by `microbundle -i test.ts -o test.js`)

exports.fn = function () {
  return Promise.resolve(1).then(function () {
    var n = arguments; // <- wrong function scope!!!
    [].forEach(function () {
      var o;
      return (o = console).log.apply(o, [].slice.call(n));
    });
  });
};
//# sourceMappingURL=test.js.map
rschristian commented 1 year ago

Looks to be the same issue as #1001

zhanghaobin commented 1 year ago

@rschristian Is there a plan to fix this issue? Currently I'm using the following workaround to avoid it:

export const fn = async (...args: unknown[]) => {
  const argsCopy = args
  await 1;
  [].forEach(() => console.log(...argsCopy))
}
rschristian commented 1 year ago

Not that I know of. I might be able to take a look later this week, but no guarantees.

donmccurdy commented 1 year ago

I'm running into a same-ish bug — async/await is transpiled out of my CommonJS builds, and (for whatever reason) the transpiled code is incorrect. I don't really want async/await transpiled at all though. Is there a browserslist config that will keep async/await in my CommonJS builds, to work around the issue? I haven't specified --target and this doesn't seem to do it:

"browserslist": [
  "node >= 20"
],

The 'modern' build works fine, but I'm trying to keep CJS support a while longer at least.

Glad to file a new bug with more details if this is unexpected.

rschristian commented 1 year ago

async/await is transpiled out of my CommonJS builds, ... I haven't specified --target and this doesn't seem to do it:

If you're targetting Node (as your browserslist config example seems to indicate), you should be specifying the target. Microbundle targets the web by default.

Add --target node to your build command and async/await will be preserved.

donmccurdy commented 1 year ago

Sorry, I'm not actually targeting Node – just an example. I am targeting Node+Web+Deno. I've tried "last 1 Chrome version" as well, and couldn't get async/await output from that. It seems like the browserslist config is being expanded (to support older devices) beyond what I've set.

A 'modern' option for CommonJS is what I'd really like to have, but anything that gets me ES7+ would be fine.

rschristian commented 1 year ago

Sorry, but in that case this is operating as intended:

https://github.com/developit/microbundle/blob/3aca5a0a8f9b0df57a1603f044fdd8e86da79ae7/src/lib/babel-custom.js#L90-L96

A 'modern' option for CommonJS is what I'd really like to have, but anything that gets me ES7+ would be fine.

Honestly, consumers should just be using the modern (ESM) builds. The cjs, umd, and esm output formats are entirely for legacy use as they intentionally target ES5. package.json#exports does allow you to gate newer CJS behind it, but as Microbundle isn't going to output multiple CJS bundles by default, I think the general position is to label CJS as a legacy build that there are incentives to get off of using.

However, this is getting a bit off topic. If you wanted to create an issue for allowing modern output of other builds you can do so, but I'm not sure it'll be addressed. As for this issue, I haven't been able to look into it, but it'll probably be a bug with our fast rest transform or the async-to-promises Babel plugin, but I think it's pretty much unmaintained.

donmccurdy commented 1 year ago

Honestly, consumers should just be using the modern (ESM) builds...

Agreed, CJS has cost a lot of my time (not Microbundle's fault). Someday I'll be very excited about Sindre'ing all my packages, but I haven't worked up the nerve quite yet.

I'll leave a comment on #618 for now.

rschristian commented 1 year ago

Yeah, I get that. The ecosystem is in a rough place, though showing some signs of improvement.

However, I think our philosophy here is going to go the other way: it's been mentioned in a few issues that eventually we might switch over the legacy builds to their more verbose, correct forms (as Babel's loose mode does cause some issues) which likely means you're not going to see a more succinct, modern CJS build, but an even larger legacy bundle.

Who knows if/when that'll happen though.

l be very excited about Sindre'ing all my packages

lol, I love that "Sindre'ing" is a verb