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

ES6-ify void-dom-elements-no-children rule and test file #1274

Merged
merged 1 commit into from
Jun 26, 2017

Conversation

dfilipidisz
Copy link
Contributor

I've seen in issue #1240 that using some new patterns from node 4 would be a nice addition.

Starting out I only updated the void-dom-elements-no-children rule and test file to use:

  • const and let instead of var
  • Arrow funtions
  • Template literals instead of [].join()
  • Some minor unification of formatting

I've also seen a comment in the rule file that using Set would be nice, which is also available with node 4. Can I make that conversion too?

Is this PR okay, and if so, is there a preferred way I should continue with updating?

Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

As for updating the rest, I'd suggest automating the conversion in some way and also enforcing it with appropriate eslint rules where possible. I'd also recommend one commit per transfrormation across all files (e.g. one commit that changes var to const or let and enforces it via an eslint rule for all files). These are okay as separate PRs as well, which might help them get merged faster.

Some changes can be automated by enabling the rule and running eslint --fix, while other changes can be automated via codemod. (More info on this: https://medium.com/airbnb-engineering/turbocharged-javascript-refactoring-with-codemods-b0cae8b326b9).

@dfilipidisz
Copy link
Contributor Author

Thanks for the review and directions on how to proceed! I'll look into how to utilize codemods for this and will keep each transformation in separate PRs.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! and yes, using Map/Set is totally OK now that we require node 4+.

@ljharb ljharb merged commit 1c15d26 into jsx-eslint:master Jun 26, 2017
@dfilipidisz dfilipidisz deleted the issue-1240 branch June 26, 2017 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants