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

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

Open
zhanghaobin opened this issue Apr 3, 2023 · 9 comments
Open

Comments

@zhanghaobin
Copy link

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
Copy link
Collaborator

Looks to be the same issue as #1001

@zhanghaobin
Copy link
Author

@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
Copy link
Collaborator

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

@donmccurdy
Copy link
Contributor

donmccurdy commented Oct 26, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

donmccurdy commented Oct 27, 2023

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
Copy link
Collaborator

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

!customOptions.modern &&
!isNodeTarget && {
name: 'babel-plugin-transform-async-to-promises',
inlineHelpers: true,
externalHelpers: false,
minify: true,
},

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
Copy link
Contributor

donmccurdy commented Oct 27, 2023

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants