Commonjs with core-js treeshakability regression

[TrySound]

Ref atlassian/react-beautiful-dnd#446

After upgrading from rollup 56 to rollup 58 I got umd bundle size is significantly increased. Diff shows me this
Here's part of the diff

+  var _toInteger$1 = /*#__PURE__*/ Object.freeze({
+    default: _toInteger,
+    __moduleExports: _toInteger
+  });
+
   // 7.2.1 RequireObjectCoercible(argument)
   var _defined = function(it) {
     if (it == undefined) throw TypeError("Can't call method on  " + it);
     return it;
   };

+  var _defined$1 = /*#__PURE__*/ Object.freeze({
+    default: _defined,
+    __moduleExports: _defined
+  });
+
+  var toInteger = (_toInteger$1 && _toInteger) || _toInteger$1;
+
+  var defined = (_defined$1 && _defined) || _defined$1;
+
   // true  -> String#at
   // false -> String#codePointAt
   var _stringAt = function(TO_STRING) {
     return function(that, pos) {
-      var s = String(_defined(that));
-      var i = _toInteger(pos);
+      var s = String(defined(that));
+      var i = toInteger(pos);
       var l = s.length;
       var a, b;
       if (i < 0 || i >= l) return TO_STRING ? "" : undefined;
@@ -81,8 +95,18 @@
     };
   };

Here's full diff https://gist.github.com/TrySound/8f8dd22d2398f02da3ac3522575e126b

[guybedford]

Thanks for posting. Would it be possible to try and create a simple replication here using a reduced input?

[guybedford]

I think I just hit a similar scenario here myself, which seems like a 0.58 regression to me. I can try to isolate my case as well when I get some time.

[guybedford]

As far as I can tell, the issue is that the namespace pattern x && x.default || x is not being treeshaken into just x.default?

Here's a replication, although it also seems to be the case in 0.56.5:

https://rollupjs.org/repl?version=0.56.5&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG1hdGhzJTIwZnJvbSUyMCcuJTJGbWF0aHMuanMnJTNCJTVDbiU1Q25leHBvcnQlMjBkZWZhdWx0JTIwbWF0aHMlMjAlMjYlMjYlMjBtYXRocy5kZWZhdWx0JTIwJTdDJTdDJTIwbWF0aHMlM0IlMjIlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybWF0aHMuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwZGVmYXVsdCUyMCdtYXRocyclM0IlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

I believe supporting this simplification relies on us tracking values and truthiness though.

Did you try perhaps downgrading the CommonJS plugin? As this may be in that output somehow.

[lukastaegert]

There might be some relation to rollup/rollup-plugin-node-resolve#155 you might want to check out. I.e., is it possible there is some interaction with an update rollup-plugin-node-resolve?

[kzc]

Cannot reproduce. Specifically cannot get any version of Rollup (56.x or 58.x) to drop the code in question.

@TrySound could you post a gist of a --no-treeshake version of the Rollup output?

[guybedford]

As far as I can tell this came up again in #2159, and a fix has been provided at rollup/rollup-plugin-commonjs#313. @TrySound if you can try that patch, would be interesting to hear if it resolves your issue.

[TrySound]

@guybedford Yeah, seems like the problem is solved with that patch.