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

Implement rule equivalent to "show-errors" in eslint-plugin-flowtype-errors? #166

Closed
gajus opened this issue Dec 1, 2016 · 13 comments
Closed

Comments

@gajus
Copy link
Owner

gajus commented Dec 1, 2016

Would there be interest to implement a rule equivalent to "show-errors" in https://github.com/amilajack/eslint-plugin-flowtype-errors?

There appears to be a measurable demand (https://github.com/amilajack/eslint-plugin-flowtype-errors/graphs/contributors) for implementing Flow error reporting via ESLint.

Pros:

  • Enables easy use of Flow via a familiar interface
  • It would potentially attract more people to use this plugin (= more contributors)

Cons:

  • Complicates eslint-plugin-flowtype setup
    Flow (flow-bin) would need to be installed either as a dependency of eslint-plugin-flowtype (making this package a lot heavier) or user would need to install Flow separately (the approach that eslint-plugin-flowtype-errors is taking).
  • ... ?
@gajus
Copy link
Owner Author

gajus commented Dec 1, 2016

@danharper thoughts?

@danharper
Copy link
Collaborator

Are you proposing merging the plugins if they'd be happy with it? Wouldn't want to duplicate effort otherwise.

It looks like the plugin no longer installs Flow itself, and relies on a peerDep instead.

@gajus
Copy link
Owner Author

gajus commented Dec 1, 2016

Adding to my original comment:

My personal view is that this feature is outside of scope for this package. Mixing type checking information with linting information introduces an information overload: what used to be syntactic and stylistic check adds an unexpected component of type checking. It is my view that the most efficient workflow is: type check, test, lint.

Furthermore, all reasonably popular IDEs that I know of (Sublime, Atom, vscode, intellij) that support ESLint also have a plugin for Flow.

@danharper
Copy link
Collaborator

Tbf, I feel the same way. Let's keep not worry about it then?

@gajus
Copy link
Owner Author

gajus commented Dec 1, 2016

@danharper, @thejameskyle approached me via email suggesting the idea of merging the two. I assume he approached @amilajack with an equivalent proposal.

My answer was that that I have quoted.

@danharper
Copy link
Collaborator

If there is demand for it though, I'd rather discovery & configuration be easy for users, so merging would make sense.

@gajus
Copy link
Owner Author

gajus commented Dec 1, 2016

If there is demand for it though, I'd rather discovery & configuration be easy for users, so merging would make sense.

Thats what I am thinking too. The biggest pro is that it would potentially attract more people to use this plugin. More people using the plugin = more visibility = more potential contributors to the code base.

@amilajack
Copy link
Collaborator

amilajack commented Dec 1, 2016

I assume he approached @amilajack with an equivalent proposal.

I was not approached about this.

My thoughts on this are that it should be merged but it should be disabled by default. That would make it easy for editors without flow support to have support. Also I'm not sure if this is just me but when using IDE/text editor plugins for flow, I've noticed high CPU usage (all cores at 100%) on my MacBook Pro 2014. I've tried Nuclide, flow-ide, and ide-flow

Also if this project decides to merge eslint-plugin-flowtype, I would recommend doing so after these issues are fixed:

So I would say:

  • Pros
    • Adds support for editors without flow support
    • Better performance (not confirmed yet)
  • Cons:
    • Most flow users already have IDE support (therefore, show-errors should be disabled by default)

@danharper
Copy link
Collaborator

I've noticed high CPU usage (all cores at 100%)

I used to get it a lot with IntelliJ, however it's calmed down recently. Still happens every now and then, though.

@gajus
Copy link
Owner Author

gajus commented Dec 5, 2016

@amilajack Thank you for your follow up.

Also if this project decides to merge eslint-plugin-flowtype, I would recommend doing so after these issues are fixed:

Please raise a PR incorporating the changes, when you will you are ready.

I'd be happy to make you a collaborator in the project. Most of the work recently has been carried out by @danharper, I'd like you to collaborate on future releases. That is, if you are interested?

@amilajack
Copy link
Collaborator

I'm not that great with regex but I'll help with anything that I can. Also what do you think about keeping the project separate? I think eslint-plugin-flowtype should extend eslint-plugin-flowtype-errors. That way we can keep the build steps separate and avoid complexity.

Thoughts?

@gajus
Copy link
Owner Author

gajus commented Dec 5, 2016

I think eslint-plugin-flowtype should extend eslint-plugin-flowtype-errors. That way we can keep the build steps separate and avoid complexity.

Absolutely.

I will have a look into whats the best way to extend an existing plugin in this way. Unless you are already familiar with the steps?

@amilajack
Copy link
Collaborator

No worries. I'll make a PR for this. Just wanted to confirm extending the plugin is the right way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants