fix: separate detectBuildsystems and listFrameworks #4766
Conversation
jobala
changed the title
jobala
changed the title
| // only if we find a root package.json we know this is a javascript workspace | ||
| if (Object.keys(context.rootPackageJson).length > 0) { | ||
| info.packageManager = await detectPackageManager(context.projectDir, context.rootDir) | ||
| info.buildSystems = await detectBuildSystems(context.projectDir, context.rootDir) |
There was a problem hiding this comment.
I would still rather opt for having an additional try catch block (to avoid crashing the package manager detection)
There was a problem hiding this comment.
I opted to remove the try...catch block here because I am handling invalid package.json file gracefully. In what other scenario is build system detection likely to fail?
There was a problem hiding this comment.
read package up for example? There might be always something that's not in your control and as this is such a critical step in our build process we should just wrap it a catch potential errors.
Even if now nothing is breaking it might be in the future and then everybody forgot about this use case. Adding here a try catch does not hurt but hardens our resilience
Co-authored-by: Lukas Holzer <lukas.holzer@netlify.com>
Co-authored-by: Lukas Holzer <lukas.holzer@netlify.com>
| const info: Info = { frameworks, buildSystems } | ||
| const info: Info = { frameworks } | ||
|
|
||
| info.buildSystems = await detectBuildSystems(context.projectDir, context.rootDir) |
There was a problem hiding this comment.
can you wrap this in a try catch please?
lukasholzer
left a comment
lukasholzer
left a comment
There was a problem hiding this comment.
Thanks for try/catching it ;)
LGTM!
🎉 Thanks for submitting a pull request! 🎉
Summary
Removes
detectBuildSystemsfromlistFrameworkstry...catchblock so that it executes even whenlistFrameworkserrors out.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.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)