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

Add emoji to server status messages #3000

Closed
wants to merge 3 commits into from
Closed

Add emoji to server status messages #3000

wants to merge 3 commits into from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Dec 12, 2016

This change adds emoji to the various server status messages printed by flow. These messages are usually seen in square brackets with labels like "parsing" or "merging inference". The new behavior is configurable via the emoji option in .flowconfig, but will only be used in non-Windows platforms. It's disabled by default, so to opt-in you must add:

[options]
emoji=true

flow-emoji

Test plan (on a Mac):

  • make test
  • Started flow with an empty flowconfig and didn't see emoji.
  • Started flow with a .flowconfig that had emoji=true under [options], and saw emoji.
  • Made a js change after the flow server had started, ran flow again, and saw the appropriate emoji given the config.
  • Ran flow | cat with emoji enabled and didn't see any emoji.
  • make js and node -p 'require("./bin/flow.js").checkContent("-", "// @flow\n(1: string);")' - this showed the correct error.

@samwgoldman
Copy link
Member

samwgoldman commented Dec 13, 2016

Should we make emoji the default? ❤️ if yes, 🎉 if no.

@gabelevi
Copy link
Contributor

Regarding the default - how widespread is emoji support? That would be my main concern with turning this on by default. @vkurchatkin - what concerns do you have?

@vkurchatkin
Copy link
Contributor

@gabelevi I think this change provides little to none benefit, but introduces a risk of breaking someone's setup

@zertosh
Copy link
Member Author

zertosh commented Dec 13, 2016

@Daniel15 what's emoji support like in Windows?

This change adds emoji to the various server status messages printed by flow. These messages are usually seen in square brackets with labels like "parsing" or "merging inference". The new behavior is configurable via the "emoji" option in `.flowconfig`. It's enabled by default, so to opt-out you must add:

```
[options]
emoji=false
```
@zertosh
Copy link
Member Author

zertosh commented Dec 18, 2016

I've changed my mind and made the current behavior (no emoji) the default. So it's now opt-in. Also added a line in the docs about it.

@Daniel15
Copy link
Member

what's emoji support like in Windows?

Not too good, unfortunately. They work in some configurations (for example, they work for me in Cmder), but they appear small and black-and-white. It seems like the new colourful Emoji can't be rendered in command-line apps yet. It seems like vanilla cmd.exe/conhost.exe don't support Emoji at all. See yarnpkg/yarn#405

@zertosh
Copy link
Member Author

zertosh commented Dec 19, 2016

Thanks @Daniel15!

@samwgoldman after seeing yarnpkg/yarn/issues/405, I'll try to do something similar - I'll add a TTY check and exclude windows.

@facebook-github-bot
Copy link
Contributor

@samwgoldman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@abuseofnotation
Copy link

hm, this does not seem to work.

I specify

[options]
emoji=true

I get

Unsupported option specified! (emoji)

@zertosh
Copy link
Member Author

zertosh commented Feb 20, 2017

This was released in Flow 0.38.0 (https://github.com/facebook/flow/releases/tag/v0.38.0)

@zertosh zertosh deleted the emoji branch March 6, 2017 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants