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

Fixed an issue with logging non-stringified Buffers in case of publish errors #760

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

Andarist
Copy link
Member

fixes #737

The problem is that all functions in @changesets/logger accept anys and I think that should be fixed cause it can lead to mistakes like this.

This could be fixed with this patch:

patch
diff --git a/packages/logger/src/index.ts b/packages/logger/src/index.ts
index d0d3f51..8992673 100644
--- a/packages/logger/src/index.ts
+++ b/packages/logger/src/index.ts
@@ -15,22 +15,22 @@ function format(args: Array<any>, customPrefix?: string) {
   );
 }
 
-export function error(...args: Array<any>) {
+export function error(...args: Array<string>) {
   console.error(format(args, chalk.red("error")));
 }
 
-export function info(...args: Array<any>) {
+export function info(...args: Array<string>) {
   console.info(format(args, chalk.cyan("info")));
 }
 
-export function log(...args: Array<any>) {
+export function log(...args: Array<string>) {
   console.log(format(args));
 }
 
-export function success(...args: Array<any>) {
+export function success(...args: Array<string>) {
   console.log(format(args, chalk.green("success")));
 }
 
-export function warn(...args: Array<any>) {
+export function warn(...args: Array<string>) {
   console.warn(format(args, chalk.yellow("warn")));
 }

However, this would require further changes in the codebase and I didn't want to make them right now. Mainly all .catched errors would become harder to pass to logger.error:

.catch((err: unknown) => {
error(err);
});

On the other hand, this is somewhat desirable, because at least we'd have to make the explicit conversion at those call sites. Thoughts?

@changeset-bot
Copy link

changeset-bot bot commented Feb 23, 2022

🦋 Changeset detected

Latest commit: a40b203

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a40b203:

Sandbox Source
Vanilla Configuration

@emmatown
Copy link
Member

On the other hand, this is somewhat desirable, because at least we'd have to make the explicit conversion at those call sites. Thoughts?

Yeah, I think being explicit about it would be nice

@Andarist Andarist merged commit 5a2a59a into main Feb 24, 2022
@Andarist Andarist deleted the fix/737 branch February 24, 2022 11:11
@github-actions github-actions bot mentioned this pull request Feb 24, 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.

Error messages are not human readable
2 participants