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

Fix handling of multiple compilation errors #1011

Merged
merged 1 commit into from Nov 23, 2021

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Nov 22, 2021

Currently, when compiling multiple bundles, the first error is reported, and the rest are reported in the air (after the command is reported as finalized).

This patch fixes, so all errors are acknowledged, and that command reports final error when compilations of all bundles finalizes

@medikoo medikoo changed the title Fix handling of multiple errors Fix handling of multiple compilation errors Nov 22, 2021
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Any example of the CLI output?

lib/compile.js Show resolved Hide resolved
@medikoo
Copy link
Contributor Author

medikoo commented Nov 22, 2021

Any example of the CLI output?

With modern logs for Framework v3 release (which I plan to propose with next PR), without this change, issue exposes as:

Capture d’écran 2021-10-14 à 10 32 59

(Note the another stats error log, after command reported the final error).

After this change, all the stats logged will appear before final command error output

Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

which I plan to propose with next PR

How many did you plan to propose? :D
I'm joking, thanks for your contributions!

@j0k3r j0k3r requested a review from vicary November 22, 2021 18:19
@@ -87,7 +87,7 @@ function webpackCompile(config, logStats, ServerlessError) {
_.forEach(stats, compileStats => {
logStats(compileStats);
if (compileStats.hasErrors()) {
throw new ServerlessError('Webpack compilation error, see stats above');
throw _.assign(new ServerlessError('Webpack compilation error, see stats above'), { stats: compileStats });
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually a change request, but rather grabbing the chance to discuss with people who cares about the project.

When I am experimenting the reverting of MultiCompile/ combined entries approach, I can't help but think of a complete rewrite of this project with TypeScript. The monkey patching approach brought down from Serverless V1 makes debugging and error tracing a bit tricky in many cases.

The effort is not small though, do you think it's worth doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vicary thanks for comment, but do you see this discussion specific to this contribution?

This PR is about grouping all compilation errors into one error, and not maintaining a situation where first one is reported, and others jump out aside (after Framework reports that command processing is finalized)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's more a global discussion not specific to your contribution

Copy link
Member

Choose a reason for hiding this comment

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

Seeing that you're adding stats to the ServerlessError object, I just expanded the topic a bit.

But definitely not a blocker for this issue, just general discussion.

@j0k3r j0k3r changed the title Fix handling of multiple compilation errors Fix handling of multiple compilation errors Nov 23, 2021
@j0k3r j0k3r merged commit a8688ce into serverless-heaven:master Nov 23, 2021
@j0k3r j0k3r added this to the 5.7.0 milestone Dec 1, 2021
@j0k3r j0k3r modified the milestones: 5.7.0, 5.6.1 Jan 26, 2022
@j0k3r j0k3r mentioned this pull request Jan 26, 2022
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.

None yet

3 participants