fix(eachModule): shortcircuit the iteration when callback returns true

[fkleuver]

Will make eachModule behave consistently with loader-default and how the public API describes it.

[fkleuver]

@jods4 I added a clarifying comment.

loader-default does also add a try/catch block around it though, but I'm not sure if that's meant to catch the error when defined has no indexer or to catch an error in the callback. I assumed the former since the error is completely suppressed and loader-webpack had no try/catch.

Do you know if this difference is intentional or should loader-webpack perhaps also have a try/catch?

[jods4]

My guess is that the try/catch is to prevent a crash caused by the callback. So strictly speaking I think it should be added if we want the loader to behave exactly the same as other loaders, even when facing failures.

[fkleuver]

I know it's nit-picky but I believe the key difference lies in "break when return true" being a documented feature, whereas the try/catch is not. I honestly think the try/catch should not be there in the other loader (or at least not in that form) in the first place. Suppressing errors completely is hardly ever a good idea.

So however unlikely it might be, there could be production code out there relying on being able to catch their own errors caused by the callback. Then if the loader suddenly starts suppressing those errors after a patch/minor version, that's potentially a very hard to track down breaking change.

Does this logic make sense to you / is this realistic, or am I overthinking it? Just trying to stick with the general "no breaking changes" mantra here.

[jods4]

I 100% agree that it would be best not to catch those exceptions in the first place... but the fact is other loaders do and we can't change that as it would be a breaking change.

As far as loader-webpack goes... today it doesn't catch exceptions and it's also a breaking change to catch them... although very unlikely to break anything.

On the other hand, exposing the same behaviour as other loaders is a valid argument. Library authors don't test their code with every possible loader. People port applications from older builds (e.g. requireJS) to Webpack and they shouldn't be exposed to subtle crashes.

So my thinking is that we should probably match observable behaviours. In theory any change is a breaking one but in this case it's highly unlikely that someone threw an exception inside the callback with the explicit intention to catch it from outside. Even more so since these APIs are very seldom used outside of core Aurelia libraries.

@EisenbergEffect What do you think?

[EisenbergEffect]

@jods4 I strongly favor moving in a direction that aligns our plugin with loader-webpack. If it's a slight breaking change, I think that's ok in this case. I'd want to test it with a few setups, just to make sure though.

[EisenbergEffect]

We can do a major version rev to address this.

[fkleuver]

So make this change a patch, and do the try/catch in a separate change as a major?

[EisenbergEffect]

I wouldn't bother adding the try/catch. If I recall, it's there in other loaders due to some issues that can happen with iterating those particular module registries. It probably doesn't apply to webpack.