`object-shorthand` rule doesn't play well with spread
MoOx opened this issue · 30 comments
options = {
...options,
}this code trigger the an error with the rule object-shorthand: [2, "always"].
It sounds like a bug.
Not a bug. ESLint will throw a hissy fit if it encounters a SpreadProperty so it needs to be turned into another node type and the only one it wont get upset at it Property. So it's then turned into a shorthand property. This will likely be one of those unavoidable issues that will need to be added to the README's list of Known Issues.
Oh I see. Thanks for the clear explanation.
How about doing something like react-bootstrap/react-bootstrap#860 is proposing where this plugin replaces the rule? Then using the .babelrc file you could fall back to the default eslint behavior if the rest spread rule is not enabled.
How’s it supposed to replace the rules?
On Thu, Jun 18, 2015 at 8:10 PM, Matt Smith notifications@github.com
wrote:
How about doing something like react-bootstrap/react-bootstrap#860 is proposing where this plugin replaces the rule? Then using the .babelrc file you could fall back to the default eslint behavior if the rest spread rule is not enabled.
Reply to this email directly or view it on GitHub:
#131 (comment)
That's what I assume this is doing: https://github.com/react-bootstrap/react-bootstrap/blob/internal-eslint-rules/tools/eslint-rules/index.js#L5
@jquense correct me if I'm wrong in that assumption.
It does but the parser has no access to anything like that.
On Thu, Jun 18, 2015 at 8:14 PM, Matt Smith notifications@github.com
wrote:
That's what I assume this is doing: https://github.com/react-bootstrap/react-bootstrap/blob/internal-eslint-rules/tools/eslint-rules/index.js#L5
@jquense correct me if I'm wrong in that assumption.
Reply to this email directly or view it on GitHub:
#131 (comment)
it doesn't replace them, its a plugin that handles these cases better then the built in rules. So they can be used instead of the built in object-shorthand rule for instance.
Just wondering i guess how hard it would be to have babel-eslint also expose some rules like this. Admittedly its a maintenance burden and may not be one babel-eslint wants to handle. We are just searching for the right home for this sort of thing before creating an awkwardly named npm module for them :P
context: the linked PR is to add it internally to react-bootstrap for our own dev experience, but it also seems like the sort of thing others seem to want.
Is there some way to expose a plugin for the parser to override rules? Perhaps something like that would be a better solution?
OR
Is there a way in the .eslintrc to declare plugins with:
"plugins": [
"babel-eslint/plugin"
]
Then using CommonJS load a plugin.js file at the root of the npm package. I don't know if that's feasible today, but would allow both to reside in this repo side by side.
Having the parser monkey patch rules is a road to hell that I’m not sure anyone wants to go down.
On Thu, Jun 18, 2015 at 8:29 PM, Matt Smith notifications@github.com
wrote:
@nzakas
Is there some way to expose a plugin for the parser to override rules? Perhaps something like that would be a better solution?
OR
Is there a way in the .eslintrc to declare plugins with:"plugins": [ "babel-eslint/plugin" ]Then using CommonJS load a plugin.js file at the root of the npm package. I don't know if that's feasible today, but would allow both to reside in this repo side by side.
Reply to this email directly or view it on GitHub:
#131 (comment)
Having the parser monkey patch rules is a road to hell that I’m not sure anyone wants to go down.
definitely, which is more why we were proposing to just create a real plugin that supplies complete rule replacements for affected rules. admittedly there is a syncing with the upstream code, cost, but a good bit less terrible then monkey patching
Well I see babel as a tool that monkey patches the language to test new language features which you do really well 😉. This would just be another avenue to test out ideas for eslint specifically though. Then as rules become an actual thing they would have already been well tested and easily ported to eslint proper.
Also I'm speaking for myself here but I'm sure that some of the react-bootstrap team would be open to helping this get maintained over time. Many of us have been some of eslint's earliest adopters, and in many cases reporting issues to them pretty soon after a release. We've even tried helping to resolve those issues.
and to clarify maybe the parser package is the wrong spot for this, but maybe the babel or eslint orgs? something like: eslint-plugin-babel
None of this sounds like a good idea to me. There's zero chance we'll make Espree monkey-patchable or that we'll let a plugin replace built-in rules (which would violate the principle of least surprise).
Here's the hard truth that users of experimental features need to accept: tooling isn't going to work the way you expect it to. That's the burden you take on by using features that aren't fully-baked in your code. You are voiding your warranty by doing so. So yeah, some ESLint rules just aren't going to work correctly until a feature is standardized, and that will cause you some pain. Your choices are either a) live with the pain as a reminder that you're using something that is Not Yet Final (tm), b) try to mitigate the pain, which babel-eslint does a commendable job of doing, or c) stop using that thing.
You've chosen to create custom rules that work better with experimental syntax - that's great, part of why we make it easy to write custom rules is so you can better handle situations that are specific to your use case. So, you'll have to stop using some built-in rules and instead use those...you're still not being stopped from doing what you want.
Also, I don't monitor the babel-eslint repo. Rather than at-mentioning me in a babel-eslint issue (a closed one?), it would be better to open an issue on the ESLint repo when requesting features. At least that way I can more easily track it and point others to it in the future.
I feel like there is a misunderstanding here...
I am not suggesting that the parser replace or monkey patch rules. I think we are all just wondering if either the eslint org or the babel org would be open to hosting a third party plugin as a partner to babel-eslint. One that works like every other plugin, but assumes you are using the babel-eslint parser. A user can install that plugin and enable the rules if they wish in there .eslintrc just like they do babel-eslint. (you can see from the PR linked about the "updated" rule is just a normal eslint rule, without anything fancy and no monkey patching). no new features needed, the current plugin system already solves this problem.
The reason we brought this up is just that there is at least some demand for this judging from issues in both repos. Eslint probably benefits indirectly from having someone somewhere writing forward facing rules for upcoming features. maybe its only helpful to have something to point people to when they file out of scope issues...like they point people to babel-eslint. Babel has some stake because the parser only solves part of the problem. We can just make our own plugin to handle these cases if no one is interested; we bring it up because there are teams of people already invested to some degree in this issue.
I'd be open to having a eslint-plugin-babel repo in the babel org provided you were committed to maintaining and supporting it since I'm hands off anything that isn't core.
I don't monitor the babel-eslint repo. Rather than at-mentioning me in a babel-eslint issue (a closed one?)
@nzakas I did not mean any offense by bringing you into this conversation. I merely didn't know if it was possible to do what I was asking about with eslint. The issue may be closed but that doesn't mean that it's resolved, as this is still a problem for those wanting to use this feature and get proper linting. I'm trying to see if there is a way that babel-eslint can play nice with eslint such that a rule like this one can be feasible. I've been working on a PR to eslint that I hope will play nice with this repo and any repo that cares to be both a parser and plugin. I understand your reservations about overriding core rules, which I will do my best to respect. The underlying problem here though is that this will require changes to both projects to make such a thing feasible which means working together for a moment to arrive at a solution. Since this does need communication from both teams to agree on an api that will make this feasible the issue and conversation needs to start somewhere. If you'd feel more comfortable with me opening an issue against your project for the bits relevant you your project that's fine, but how do we keep from coordinating many conversations for this?
What I propose is a PR to eslint which allows for an .eslintrc configuration like:
"parser": "babel-eslint",
"plugins": [
"~babel-eslint/plugin"
],
Then babel-eslint could add a plugin.js file at the repo root that would allow both to reside here. Would both teams be open to that?
I'm not sure why the current solution of disabling builtin ESLint rules and using custom rules isn't sufficient? Why do changes need to be made in ESLint core to accommodate this?
I was hoping to allow a single install of babel-eslint that would have both the parser and relevant rules.
I say this cause I could see this being a reasonable desire for something like CoffeeScript, TypeScript, or XYZ parser that's ever written, where that variation of the language could have rules for that variation. Granted I don't know how well ESLint works with anything other than Babel
i'd be fine maintaining a seperate repo for the plugin in the babel org, if that's easiest. the work is fairly minimal at this point.
@mtscout6 IMO the additional complexity isn't worth it when current solutions are more than adequate.
@jquense Great. I've created eslint-plugin-babel and added you to the org. Let me know if you want anybody else added. I'd prefer if discussion was taken to that repo or the #linting channel on the Slack. Thanks everyone 😄
👍 thanks for listening, and taking the time to hear us out :)
Looks like a missed a ton haha..
So it looks like we are planning to rewrite rules that have issues in the eslint-plugin-babel repo - similar to eslint-plugin-react for jsx/react and no-unused-vars?
I had already started putting together a PR to eslint for the proposed change which is eslint/eslint#2793. If it's accepted I would think that it would be a preferable option as it would allow you to consolidate issue tracking for eslint related issues to a single repo. Also, updates for eslint would only need to occur in one repo instead of two. I understand your desire to move on with a solution that already works so if that PR isn't accepted then two repos aren't the end of the world.
From a UX point of view, this will be weird to have to use two modules to "fix" eslint parsing :/
Maybe this module can use eslint-plugin-babel as a dep so people can do plugins: -babel-eslint/plugin ?
The issue is more that there is no way of doing that way unless eslint accepts a PR to allow that.
@sebmck @jquense Seeing as it's impossible to coordinate on a repo that he doesn't own a proposed solution has been given here: eslint/eslint#2796 (comment). Essentially he suggests something like:
plugins: [ "babel-eslint" ]
parser: "eslint-plugin-babel-eslint/parser"
With that I will walk away from the situation. I don't appreciate trying to search out a solution for a problem that provides a better downstream user experience, only to receive backlash and public shaming for doing so. We are all problem solvers working to provide a better experience for our customers, fellow colleagues, and also ourselves. I would hope that others like myself can see it in that order, though it cannot be imposed.
He did not propose this. I think it's not really clear the way you explained what we want.
I added a comment to (try to) clarify what we would like to do (or at least what I think we should do).
Things should be simple. Not complicated & @mtscout6 the way you explained yourself was not straightforward (from my point of view).