[Meteor 3] Fix handling unexpected errors during build

[zodern]

Meteor 3 started silently ignoring errors during parts of the build process. This would cause meteor to think it successfully built an app even though parts of the build process failed or were skipped, causing interesting problems. It also made working on those parts of the build process difficult. I ran into this yesterday when trying to figure out why a change I made to the linker was causing an unrelated issue with dynamic imports.

This PR fixes it so any unexpected error during a build causes Meteor to crash, matching the behavior Meteor had before.

[denihs]

Hi @zodern, did you run the Tools tests locally to ensure they continue passing? I remember that when I worked to fix the tests, these parts you changed gave me a lot of work, and one simple thing you change there can affect other scenarios...

I think we should hold this PR until we fix the tools CI on release-3.0 and merge it here, just to make sure it's all green.

[zodern]

There is one new failing test - compiler-plugins.js: test:compiler plugin caching - less. In the test, an error is thrown at https://github.com/meteor/meteor/blob/a67cf288a31e3462963e797b8e4462dd95703d45/tools/isobuild/compiler-plugin.js#L974-L981

As the comment says, if that line is reached Meteor will crash by design. This PR fixes it so Meteor does crash instead of ignoring the error. There is an issue with why that line is being reached, but it is unrelated to the changes in this PR.

[netlify[bot]]

✅ Deploy Preview for v3-meteor-api-docs canceled.

Name	Link
🔨 Latest commit	2ad92b7054995607e0eba486de7ca97493021357
🔍 Latest deploy log	https://app.netlify.com/sites/v3-meteor-api-docs/deploys/668593a1ec657d0008d40594

[netlify[bot]]

✅ Deploy Preview for v3-migration-docs canceled.

Name	Link
🔨 Latest commit	2ad92b7054995607e0eba486de7ca97493021357
🔍 Latest deploy log	https://app.netlify.com/sites/v3-migration-docs/deploys/668593a1c8b63100085e81b7

[leonardoventurini]

This test is still failing, any ideas what is the unrelated issue that needs fixing? @denihs @zodern

[zodern]

I'm not sure I fully understand everything, but I think this is the current situation:

In Meteor 2, it checks for errors when a css resource is added by a compiler plugin. I'm not sure how this worked, since it seemed the code would throw an error from inside the compiler plugin, which normally would crash Meteor.

In any case, that requires fibers and in early Meteor 3 versions it completely broke css files. I temporarily removed it in https://github.com/meteor/meteor/pull/12538.

Though at that point, there was no handling for errors for css resources reported by build plugins so if there was one Meteor would crash instead of waiting for a file to be modified to retry building. To fix that test, it seems at some point code was added to silently ignore all errors during part of the build process. However, there are valid errors that should not be ignored.

This PR removes the code that silently ignored the errors and cleans up some other things. However, we still need a fix for the original issue of handling errors for css resources reported by build plugins.

I'm not very familiar with this part of the build process. I think what Meteor should do is sometime after the import scanner it should go through all of the css resources and handle any errors that were reported.

[nachocodoner]

I'm not very familiar with this part of the build process. I think what Meteor should do is sometime after the import scanner it should go through all of the css resources and handle any errors that were reported.

Alright, it's uncertain if any of us could handle this issue as is unknown for any of us. How critical do you think is it? Has any other developer reported it yet?

To tackle this problem effectively, setting up a reproduction repository would help us explore it deeply and find possible solutions.

If I've got this right, any build plugin designed for work with CSS resources and added to a Meteor 3 app would fail due to the lack of support for fibers on managing the resource. Silent it made it worse. And we need a way to handle this and any error behind. We should create a reproducible example covering the scenarios.

[zodern]

How critical do you think is it? Has any other developer reported it yet?

I've run into it before, and I'm pretty sure the error in https://github.com/zodern/melte/issues/36 is due to it. It seems like something is erroring before the build process reaches the step to replace __DYNAMIC_VERSIONS__, but Meteor is ignoring and hiding the actual error.

[zodern]

To tackle this problem effectively, setting up a reproduction repository would help us explore it deeply and find possible solutions. If I've got this right, any build plugin designed for work with CSS resources and added to a Meteor 3 app would fail due to the lack of support for fibers on managing the resource. Silent it made it worse. And we need a way to handle this and any error behind. We should create a reproducible example covering the scenarios.

There's two issues:

1. Unexpected errors during the build, which this PR fixes. You can easily reproduce it by adding a throw new Error('test') somewhere in the linker or other parts of the build process that are run inside the try ... catch block. This makes development more difficult, and there are a few places where Meteor throws errors that should not be ignored.
2. Handling the errors reported for css resources by compiler plugins. The existing test less test is a good reproduction. When we remove the code to silently ignore errors, it fails due to this error being thrown: https://github.com/meteor/meteor/pull/12675#issuecomment-1592354248. We just need to find the right place to call reportPendingErrors and stop the build if there are errors, probably sometime between running the import scanner and whatever code uses the css resources.

[JanMP]

I got ReferenceError: __DYNAMIC_VERSIONS__ is not defined without zodern:melte

I have a package with loads of React Components (some of them are HUGE) that I export from the package like this:

import React, {Suspense, lazy} from 'react'

suspend = (WrappedComponent) -> (props) ->
  <Suspense fallback={<div>Loading...</div>}><WrappedComponent {props...}/></Suspense>

export ActionButton = suspend lazy -> import('./forms/ActionButton.coffee').then (m) -> default: m.ActionButton

# [31 more components like this]

So, this is really just straightforward dynamic imports.

[denihs]

In this commit, I fixed the test compiler plugin caching - less.

This is the breakdown:

After the changes in this PR, we can throw errors from the build, and the app crashes.

With that, this specific test was failing because the app wasn't just logging the error message; it was stopping:

This error is thrown here:

https://github.com/meteor/meteor/blob/c91daf9d6ea14f15ebaeb50f0d724736768b72c2/tools/isobuild/compiler-plugin.js#L969-L985

And we get here for this test when we hit this part of the code:

https://github.com/meteor/meteor/blob/776b3c22d88481df85e8dafc6072a4090c297466/tools/isobuild/bundler.js#L1293-L1296

So my solution was to do what the comment above the throw command says and check if there are any errors before trying to get the data:

https://github.com/meteor/meteor/blob/776b3c22d88481df85e8dafc6072a4090c297466/tools/isobuild/bundler.js#L1276-L1280

What do you think about this @zodern? cc @nachocodoner @leonardoventurini

I couldn't reproduce the __DYNAMIC_VERSIONS__ issue. @JanMP, can you try to reproduce this problem locally with this branch to see if you can get anything different?

[JanMP]

I couldn't reproduce the __DYNAMIC_VERSIONS__ issue. @JanMP, can you try to reproduce this problem locally with this branch to see if you can get anything different?

I can't reproduce it anymore. I have had to keep working on the project under 2.16 and now I get a different error when I update to 3.0-rc.4 (one that shows up on the server at runtime).