developit / microbundle

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

Spread syntax in array incorrectly converts to [].concat() #945

Closed mmdevcodes closed 2 years ago

mmdevcodes commented 2 years ago

When I do:

const elements = [...document.querySelectorAll("button")];

I get:

var elements = [].concat(document.querySelectorAll("button"));

elements ends up being an array of NodeList instead of the intended array of elements.

Is there a config change I can do to avoid this?

developit commented 2 years ago

There is no config for this, Microbundle assumes spreads are Arrays in order to produce the smallest output for ES5 targets. I would recommend using [].slice.call(document.querySelectorAll("button")) in your code, since it's faster in all engines and output targets.

ivandotv commented 2 years ago

I have just run into this. Took me a couple of hours to track it. I believe this is due to the loose option in the babel config. this code:

export function test() {
  const testArr = [...new Set([1, 3, 4, 4])]
}

get compiled to:

function t() {
  var t = [].concat(new Set([1, 3, 4, 4]))
}

this happens with format:cjs

modern format does not change the code.

There is a new option in babel preset-env (docs say it should replace loose mode) https://babeljs.io/docs/en/assumptions I wonder if this could help.

mmdevcodes commented 2 years ago

Yes as mentioned here loose mode transpiles ES6 code to ES5 code that is less faithful to ES6 semantics and isn't generally recommended to use. Removing it or at the very using the assumptions option @ivandotv mentioned would generate less errors.

rschristian commented 2 years ago

Hm, there might be a misunderstanding here? This is working as intended, there's nothing to "help" or "fix".

Yes, Microbundle does use loose mode, and yes this might introduce edge cases, but the goal of Microbundle is the tiniest output possible for most modules. Removing loose mode isn't likely in the near future; when modern output becomes consumed more often than not, it may make sense to allow the ES5 to grow to the bigger, more correct form, as it'll rarely ever be used, but we're not there yet.

loose mode transpiles ES6 code to ES5 code that is less faithful to ES6 semantics and isn't generally recommended to use.

That 7 year old link might not recommend them, but far more often than not, loose mode lines up pretty well with what users are actually writing and expecting. document.all is a lovely example of bloat caused by correct transformations for cases that are a non-issue to most users.

assumptions just replaces the various loose options in plugins. Switching over to it would have no effect, as we'd aim for the very same behavior, if not make even more assumptions creating tinier output.

rschristian commented 2 years ago

Going to close this out as there's nothing actionable here and the suggested replacement code should resolve the issue.

make-github-pseudonymous-again commented 6 months ago

Am I understanding correctly that Array.from is missing in ES5 and that we want to avoid bringing in a polyfill?

rschristian commented 6 months ago

Sorta, but defensively calling Array.from on any spread in the off chance it's not an array would be pretty wasteful.

make-github-pseudonymous-again commented 6 months ago

wasteful

1 character for [...x] with Array.from(x) instead of [].concat(x), and 12 characters for [x, ...y, z] with [x].concat(Array.from(y),[z]) instead of [x].concat(y,[z])?

rschristian commented 6 months ago

[].concat() should perform significantly better in all engines ([].slice.call() even better yet). Array.from() is hardly the most problematic API, but like many ES6 APIs, the performance leaves something to be desired.