Avoid using non-ASCII characters in exception messages

jaydenseric / graphql-upload

Middleware and a scalar Upload to add support for GraphQL multipart requests (file uploads via queries and mutations) to various Node.js GraphQL servers.

https://npm.im/graphql-upload

MIT License

1.43k stars 132 forks source link

Avoid using non-ASCII characters in exception messages #205

Closed koresar closed 4 years ago

koresar commented 4 years ago

So, we bumped into the famous

"Missing multipart field \u2018operations\u2019 (https://github.com/jaydenseric/graphql-multipart-request-spec)."

The reverse apostrophe and the mirrored apostrophe do not look well in my server logs terminal.

Best practice for backend engineering:

use ASCII only characters in log messages,
this means to use ASCII only characters in error (exception) messages.

Should I submit a PR to use ' or " or else instead of the \u2018 and \u2019?

jaydenseric commented 4 years ago

Thanks for the report, although I have mixed feelings about it.

I've never seen any of these characters display incorrectly in any of my terminals, don't most support UTF-8?

I'm not so sure it's "best practice" to only use english characters in error messages; what if you're Chinese, can't speak english, and need to write an error message?

I think we should hold environments to a higher standard, rather than limit ourselves to 1960s technology forever.

That said, if this is a widespread problem with terminals (is it?) we can look at replacing the typographically correct quotes with something simpler. One option that is becoming popular when referring to specific code is backticks (``).

koresar commented 4 years ago

I'm pretty sure every environment out there supports UTF-8 nowadays.

The root of my issue is unknown. I see the error message in Slack. Slack receives it from a python2 Lambda. Lambda receives it from AWS CloudWatch Logs. AWS CWL receives it form awslogs (I think) process within our EC2. The log file on that virtual machine is generated by bunyan node.js module.

It is very hard to find out which of the chain links has messed up.

jaydenseric commented 4 years ago

It's good we have this issue for the record, I'll keep the suggestion in mind.

I'll close this issue as I don't intend to action it right now. There is the possibility that changing the error message strings now will be a breaking change for anyone matching on them, as debatable as that practice may be. Also, if we change the approach for this package, it would make sense to also update all the other packages I maintain accordingly — something I don't really have the time for right now.

Other people feel free to chime in; we can reopen this issue if anything significant comes to light. @koresar I’m curious to hear what was messing up your quotes if you ever find out!