Conversation
|
Potential future steps:
|
npm-react/addons.js
Outdated
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
That isn't actually. My bad regexp codemod caught it, and I figured it wouldn't hurt to change it. Let me know if you want me to revert it.
There was a problem hiding this comment.
I guess let's keep it as single quotes in the require but use double quotes for the outer string so we don't need the escapes.
There was a problem hiding this comment.
We don't support let internally at FB yet so this can't go in like this.
There was a problem hiding this comment.
It's been brought up multiple times that we shouldn't be running these tests internally anyways, so I'll look into that issue before we merge this.
There was a problem hiding this comment.
We already don't run the tests internally. But we still lint internally and I wouldn't be surprised if we end up choking on let. Regardless though, let's not introduce the first let here.
|
This looks pretty good overall. |
Removes all `/*eslint-disable no-unused-expressions */` comments. > Instead of disabling this rule, maybe we chould use the `void` > operator on each place that triggers it (which this rule's docs claim > doesn't warn). Like `void frag[key];`. facebook#3985 (comment) @spicyj
> Does (x) => x * 2 work here? If so, let's do that – but otherwise this > is okay. facebook#3985 (comment) @spicyj
There was a problem hiding this comment.
Any difference between true and false here? If not let's be consistent (I don't care what the value is if it works)
There was a problem hiding this comment.
To configure global variables inside of a configuration file, use the globals key and indicate the global variables you want to use. Set each global variable name equal to
trueto allow the variable to be overwritten orfalseto disallow overwriting.
http://eslint.org/docs/user-guide/configuring.html
EDIT: I can comment something to that effect in the eslintrc if you want.
There was a problem hiding this comment.
Ah no, I just didn't realize there was a difference. I might have known back when I first did this (I think there's a test that toggles __DEV__)
> For this one, we could just do displayName: 'NamedComponent', inside > the class spec and drop the var assignment. facebook#3985 (comment) @spicyj
|
I think I fixed everything except https://github.com/facebook/react/pull/3985/files#r31360729 , which I'd like an opinion on, and the trailing comma issue, which I'll fix in a later PR, after this one is merged. I could revert the comma changes I made here and disable the rule right now, but there's not really any point besides slightly shortening the diff. |
Alternatively, we could disable it globally, but this is the only place it triggers in our code. > facebook#3985 (diff)
There was a problem hiding this comment.
I don't think you need this now?
|
lgtm – can you fix those two things? Then feel free to squash and merge. |
Closes facebook#3971. > After facebook#3968, the next thing we should do is start linting our tests. > Historically we've ignored them due to lack of parser compatibility. > But that shouldn't be a problem anymore. We may want to integrate > https://www.npmjs.com/package/eslint-plugin-react to more aggressively > lint our JSX in tests. I understand this diff touches a lot of stuff, so I tried to keep it to a near-minimal set of changes to make eslint happy.
Closes #3971.
I understand this diff touches a lot of stuff, so I tried to keep it to
a near-minimal set of changes to make eslint happy. If you're wondering
why I changed something, let me know and I'll try to map it up to the
rule that I was fixing it for.