Skip to content

Conversation

@koba04
Copy link
Contributor

@koba04 koba04 commented Mar 15, 2015

react is using ESLint for lint now, so I removed the codes relating to jshint.

In addition, I refactored some DOM components, because I found the below comment.

https://github.com/facebook/react/blob/master/src/browser/ui/dom/components/ReactDOMForm.js#L35-L37

    // TODO: Instead of using `ReactDOM` directly, we should use JSX. However,
    // `jshint` fails to parse JSX so in order for linting to work in the open
    // source repo, we need to just use `ReactDOM.form`.

Are these changes unnecessary?

@chicoxyzzy
Copy link
Contributor

#3123 (comment)
BTW don't know if FB still use jshint internally

@bgw
Copy link
Contributor

bgw commented Jun 2, 2015

@koba04 We've got full eslint coverage now, so I disabled jshint for react at FB internally. Do you still have time/interest to work on this? Can you rebase it?

@koba04
Copy link
Contributor Author

koba04 commented Jun 3, 2015

@PiPeep
Thank you, I've rebased it!

I didn't remove it from vendor/browser-transforms.js because I found this comment.
#3834 (comment)

Also,

// TODO: Instead of using ReactDOM directly, we should use JSX. However,
// jshint fails to parse JSX so in order for linting to work in the open
// source repo, we need to just use ReactDOM.form.

Should I convert to use JSX?
JSX needs to have React.createElement in scope. I should use var React = require('ReactElement'); if I use JSX in React internal?

@zpao
Copy link
Member

zpao commented Jun 3, 2015

I didn't remove it from vendor/browser-transforms.js

You can change that one. My previous comment was about vendor in src. This one isn't synced with anything else.

Should I convert to use JSX?

No, leave those alone.

@zpao
Copy link
Member

zpao commented Jun 3, 2015

This lgtm. Any comments from you @PiPeep?

@bgw
Copy link
Contributor

bgw commented Jun 3, 2015

The commit lgtm, pending vendor/browser-transforms.js. I checked it against ag jshint -p .eslintignore, and it seems to cover everything.

@zpao, are we simply avoiding jsx altogether in the react source, excluding __tests__ and src/addons?

@zpao
Copy link
Member

zpao commented Jun 3, 2015

Currently that is correct (except we also don't use it in src/addons).

@bgw
Copy link
Contributor

bgw commented Jun 3, 2015

Oh, sorry. I think I was accidentally looking at code within comments.

@koba04
Copy link
Contributor Author

koba04 commented Jun 4, 2015

@zpao @PiPeep
Thank you for your feedback!
I removed it from vendor/browser-transforms.js.

@zpao
Copy link
Member

zpao commented Jun 4, 2015

Thanks!

zpao added a commit that referenced this pull request Jun 4, 2015
Remove the codes relating to jshint
@zpao zpao merged commit 6b79977 into facebook:master Jun 4, 2015
@koba04
Copy link
Contributor Author

koba04 commented Jun 4, 2015

Thank you!!

@koba04 koba04 deleted the remove-jshint branch June 4, 2015 02:57
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.

5 participants