Stop squashing module loading errors

[bendemboski]

We've apparently been logging, rather than propagating, these errors for 8 years...and that seems very wrong. In particular, if there are test module load errors, they just get logged to the console, but cannot be detected by the test loader and turned into test failures. I think also this causes module load errors to be harder to detect/understand when running apps.

So, stop doing that, and propagate these errors!

[bendemboski]

I believe this is a breaking change, as it propagates errors that used to be squashed, which certainly could break existing functionality (although existing functionality that was relying on underlying broken-ness). So...I guess maybe this change requires a major version bump?

[RobbieTheWagner]

@bendemboski as long as we are sure this was not an intentional choice to console.error these instead of throwing them, this is fine with me. I'm unsure if it needs to be a breaking change or not though. If the intention was always to pass the error through, I would consider this a bug fix.

[bendemboski]

@RobbieTheWagner it was introduced here. Adding an explanation here for posterity.

AMD module load errors can happen for two reasons -- because of an actual "legitimate" error loading the AMD module, or because the require() call wasn't trying to load an AMD module, but was instead trying to load a node module.

We can't perfectly detect the difference between these, and before the above change we weren't even trying -- every AMD load error was completely squashed, so if there was some legit module load error in the AMD module, rather than propagating it, we would try a node-load, which would almost certain result in a module not found error.

The above change made it so that we would detect AMD-module-not-found errors, and only in that case try to load as a node module. But in the case where it wasn't an AMD-module-not-found error, i.e. it was some legitimate AMD module loading error (like a syntax error or whatever), the above change would just log the error, rather than propagating it, meaning we changed from throwing something (even though it was a wrong/confusing thing) to throwing nothing, which IMO is worse that the behavior before the change. Misleading errors are bad, squashed errors are worse.

So if there was a reason for changing the behavior to squash these errors, it's lost to the ages, and I feel very confident that it's incorrect.

[bendemboski]

As far as breaking vs not, I'll investigate the test failure, and if it's not of concern/easily fixable, I think I'm with you that we can consider this a bug fix.

[bendemboski]

@RobbieTheWagner after rebasing on main to pick up the electron-is-dev fix, this branch is passing, so I think we're good to merge and consider this a bugfix. Agreed?

[RobbieTheWagner]

@RobbieTheWagner after rebasing on main to pick up the electron-is-dev fix, this branch is passing, so I think we're good to merge and consider this a bugfix. Agreed?

Agreed!