-
Notifications
You must be signed in to change notification settings - Fork 57
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: log error message when failing an integration build #5559
base: main
Are you sure you want to change the base?
Conversation
This pull request adds or modifies JavaScript ( |
@@ -82,7 +82,7 @@ const getIntegrationPackage = async function ({ integration: { version, dev }, c | |||
throw new Error(res.stdout) | |||
} | |||
} catch (e) { | |||
throw new Error(`Failed to build integration`) | |||
throw new Error(`Failed to build integration: ${e.message ? e.message : e}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like e.message
will most likely be multi-line output from npm run build
- does that work okay with the custom error parsing stuff that @netlify/build
has going on?
looking at that error handling I also wonder if a custom ErrorType would be useful here- it looks like stackType: 'message'
kind of does what we're attempting to do here, by using the other process' output as the error stack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cubeghost can you explain what you mean with the custom error parsing stuff? :O I don't think I'm aware of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah! I noticed a bunch of code in the src/error
directory, particularly src/error/types.ts
, and looked at the corresponding documentation. this code not only customizes the error output but also does a bunch of parsing and formatting of stack traces and such.
from what I can see from Uma's error, this error ends up unhandled so it follows the generic exception
rules, and it ends up logging two stacks, one for where we're throwing inside of @netlify/build
and a less visible one for the actual error in the underlying process.
so I think if we added a new error type specifically for integration build failures, we could customize where the error stack comes from, as well as customizing the error message
🎉 Thanks for submitting a pull request! 🎉
Summary
This will add logging to any integration building failure. Should be useful for integration creators.
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)