Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit process with error code when reporters fail #9735

Merged
merged 3 commits into from
May 27, 2024

Conversation

MonicaOlejniczak
Copy link
Contributor

↪️ Pull Request

This change modifies the process exit code when a reporter fails, ensuring that builds do not pass when something goes wrong in a reporter (e.g. generating a file).

Adding this exit code is the simplest mechanism for failing the build, as it does not require changing the existing API or introducing new APIs.

🚨 Test instructions

yarn test:integration

@@ -113,6 +113,7 @@ export default class ReporterRunner {
});
} catch (reportError) {
INTERNAL_ORIGINAL_CONSOLE.error(reportError);
process.exitCode = 1;
Copy link
Member

@devongovett devongovett May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is unexpected to do this for people using the Parcel programmatic API rather than our CLI? Should we store our own property here (rather than setting the global exit code), and then throw/emit an error from the main Parcel object that the CLI could pick up? We do already set an exit code in the CLI when an error is thrown.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just removed this catch and let the error propagate naturally?

Copy link
Contributor Author

@MonicaOlejniczak MonicaOlejniczak May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to avoid propagating the errors because not all of the calls are awaited on, and doing so could regress performance.

As for storing something that the CLI can pickup, I can try doing that but it will only get emitted at the end of the build unless the aforementioned un-awaited cases are changed.

Alternatively, I could only propagate and await on buildSuccess events as that's the main use-case anyway.

Anyways the latest commit throws any received errors just before returning the build success event. I also initialise the reporters earlier so errors are seen earlier + adding to them becomes simpler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could simplify this fixutre by using fsFixture - example of an integration test with a reporter using fsFixture:

describe('Reporter API', () => {
it('should pass the parcel version to plugins', async () => {
const dir = path.join(__dirname, 'plugin-parcel-version');
overlayFS.mkdirp(dir);
await fsFixture(overlayFS, dir)`
index.js:
export default 'Hi';
.parcelrc:
{
extends: "@parcel/config-default",
reporters: ["./reporter-plugin.js", "..."],
}
package.json:
{
"version": "1234"
}
yarn.lock:
reporter-plugin.js:
import {Reporter} from '@parcel/plugin';
import path from 'node:path';
export default new Reporter({
async report({event, options}) {
if (event.type === 'buildSuccess') {
await options.outputFS.writeFile(path.join(options.projectRoot, 'parcel-version.txt'), options.parcelVersion);
}
}
})
`;
await bundle(path.join(dir, 'index.js'), {
inputFS: overlayFS,
outputFS: overlayFS,
});
assert.equal(
await overlayFS.readFile(path.join(dir, 'parcel-version.txt')),
PARCEL_VERSION,
);
});
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially had an fs fixture, but want to test the CLI as well which requires the files to live on disk


module.exports = new Reporter({
async report({ event }) {
if (event.type === 'buildSuccess') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice to test for cases where there is a delay here

@MonicaOlejniczak MonicaOlejniczak added this pull request to the merge queue May 27, 2024
Merged via the queue into v2 with commit a249deb May 27, 2024
16 of 17 checks passed
@mischnic mischnic deleted the molejniczak/fail-reporters branch May 28, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants