fix(commonjs)!: use safe default value for ignoreTryCatch

[benmccann]

Rollup Plugin Name: @rollup/plugin-commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers: #1004

Description

There's a warning for this value in the docs saying it should normally be set to true, so it seems like the current default of false might not be a very good one

[shellscape]

I'm good with it, especially since the tests all pass, and reasoning makes sense.

Being a breaking change let's get a few more eyeballs on it.

[tjenkinson]

LGTM

This option solves a problem that came up literally last Friday!

[danielgindi]

Yes, makes sense. I think we defaulted it to false in the first place in order to avoid a breaking change for everyone. People need to know that things may change when they update to this version.

[rchl]

This caused a breaking change in this project: https://github.com/resoai/TileBoard

After updating to v21 the generated bundle changed this way:

--- /Users/rafal/workspace/TileBoard/build/scripts/app.js	Thu Oct 28 21:06:55 2021
+++ /Users/rafal/workspace/TileBoard/build/scripts/app.js	Thu Oct 28 21:07:10 2021
@@ -51135,7 +51135,7 @@
 
     (function (module, exports) {
     (function (global, factory) {
-    module.exports = factory(function() { try { return moment$1.exports; } catch(e) { } }()) ;
+    module.exports = factory(function() { try { return require('moment'); } catch(e) { } }()) ;
     }(commonjsGlobal, (function (moment) {
     moment = moment && moment.hasOwnProperty('default') ? moment['default'] : moment;
 
@@ -68072,7 +68072,7 @@
 
         function requireMoment() {
             try {
-                return moment$1.exports; // Using nw.js or browserify?
+                return require('moment'); // Using nw.js or browserify?
             } catch (e) {
                 throw new Error('Please install moment via npm. Please reference to: https://github.com/urish/angular-moment'); // Add wiki/troubleshooting section?
             }

Which all sounds like an expected change but the require is actually undefined in that first try block.

I know I can change an option and revert to the previous behavior but thought that you might also want to be aware.

BTW. That block is a part of an old Chart.js v2.9.4 release (https://github.com/chartjs/Chart.js/commits/2.9).

[shellscape]

This PR was released as a new major version, and the breaking change is noted in the PR.

[rchl]

That's fine. It was just not clear why it broke and whether it's expected that it did.

[danielgindi]

Happened to me too with an old module, but since the only dep with a major version change that was updated was the commonjs plugin- it took only a few minutes to figure out where the problem was.
As old modules will update their syntax or packing pipeline, or simply phase out- these issues will occur less. Eventually the commonjs plugin may not be needed at all 😉

[lukastaegert]

In hindsight I think this should only apply for external requires. Also, this will produce slightly invalid code when the output format is not CommonJS (well, it will still work as there is the try-catch to catch the missing require variable, but hey). Unless there is concern, I will add a check that this only happens for external requires and inline non-external ones in #1038 as this will also help to solve some discussions there.

[danielgindi]

In hindsight I think this should only apply for external requires. Also, this will produce slightly invalid code when the output format is not CommonJS (well, it will still work as there is the try-catch to catch the missing require variable, but hey). Unless there is concern, I will add a check that this only happens for external requires and inline non-external ones in #1038 as this will also help to solve some discussions there.

Yes it is actually meant for external requires