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

Migrate to using neo-blessed as underlying blessed renderer. #270

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

parkerziegler
Copy link
Contributor

This PR migrates us to using neo-blessed 🎉, an actively maintained fork of blessed. After doing some experimentation to look at a full rewrite with react-blessed or ink, I found significant drawbacks to each.

With react-blessed, there are some weird behaviors with some of the core styling APIs of blessed (for example, using padding breaks layout entirely). Moreover, the attributes supplied to JSX nodes in react-blessed just desugar to calls to blessed anyways, so we don't get any underlying enhancements for what would be a considerable migration.

ink isn't really in the same mold as blessed and doesn't allow you to commandeer the terminal screen, so it feels like it'd be too much of a departure from what we currently have to justify the migration. We should definitely consider it for future CLIs tho!

This PR also adds Prettier to the codebase. We use ESLint to run Prettier, so we shouldn't get any ESLint errors with this setup. We also expose a format script for users to run (assuming they don't already have their editor configured to format on save) and a format:check script that we run in CI to ensure formatting is consistent across PRs. There are no code changes in this PR, so any changes you see are purely Prettier rewrites.

@parkerziegler
Copy link
Contributor Author

Looks from the logs like we might need to deprecate support for Node 6 😱: https://travis-ci.org/FormidableLabs/webpack-dashboard/jobs/492471323. @ryan-roemer let me know how you feel about that. With 8 in LTS and 6 headed for EOL in April, I feel pretty confident in removing that from our CI checks and then slapping a big ol' warning at the top of the README.

@ryan-roemer
Copy link
Member

Yep, go ahead and remove node six support

@parkerziegler parkerziegler merged commit b435d6c into master Feb 14, 2019
@parkerziegler parkerziegler deleted the task/migrate-neo-blessed branch February 14, 2019 17:40
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.

2 participants