fix(cli): properly handle errors #217
Conversation
- Handle all errors centrally and properly set process exit code - Use console.warn where appropriate to funnel messaged to stderr - Do not ignore errors when loading config file
GeorgeTaveras1231
force-pushed
the
master
branch
from
040c03d
to
9b10ede
Compare
This stops ignoring errors generated by the sass compiler (as well as any other potential errors generated during the process of writing to disk). Instead, errors are now always thrown to the top of the stack, they are logged, and properly set the exit code. Any errors that include file info are assumed to be sass errors, and some additional formatting applied to improve debuggability.
This also changes the TS target to ES2022 to support the second argument of the Error constructor.
| }); | ||
|
|
||
| expect(fs.writeFileSync).toHaveBeenCalledTimes(6); | ||
| expect(fs.writeFileSync).toHaveBeenCalledTimes(9); |
There was a problem hiding this comment.
This number is higher because more files are now compiled, since the tool forces you to handle build errors. This is also why setting the aliases and aliasPrefix was required for this test.
|
|
||
| expect(result).toContain("No files found."); | ||
| expect(result.stderr.toString()).toContain("No files found."); |
There was a problem hiding this comment.
@skovy this was needed because of the changes to use console.warn where appropriate. NodeJS sends console.warn messages to stderr (appropriately) -- additionally execSync doesn't expose messages in stderr of the downstream process. spawnSync however does let you inspect stdout and stderr
There was a problem hiding this comment.
Got it, that makes sense. While here and making this change, can we make them all spawnSync for consistency? and it sounds like it supports more functionality which we need here so others might need this in the future as well
|
@skovy I've fixed the lint check. Let me know if I should fix anything else. |
|
should this be considered a breaking change? if some peoples builds are technically passing today, this could cause them to start failing? |
|
@skovy that's a good question - I would leave it up to you to decide ultimately. In the past I've chosen both of the options you have (push as a breaking change, or push as a patch). Its totally reasonable to push this as a breaking change and it is the safer path. I'm fine with either approach 😌 |
|
My concern with merging this right now is that it sounds like without first finding a solution in #220, this will start to error for people without a fix? I think it makes most sense if we first merge a solution to the other problem, then this PR? |
Details
I decided to open this PR after noticing some subtle issues in my build pipeline. I noticed that the typed-scss-modules build was silently failing - it couldn't load the config file because I had a broken import inside of it. Additionally, it continue trying to generate types after not being able to load the config, and it encountered another issue - it wasn't able to generate the types because it could not properly parse the sass files (because the configs were broken).
Example error:
TODO
sass.renderSyncAdditional changes
causeoption is supported in Node 16, but the TS types are a bit delayed (Error Constructor does not accept more than one argument microsoft/TypeScript#49535)