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

Updated eslint packages #426

Merged

Conversation

reconstructions
Copy link
Contributor

@reconstructions reconstructions commented Nov 30, 2017

Updating package for eslint-config-shakacode:

I updated the eslint-config-shakacode package from version ^14.1.1 to 16.0.0-rc.3. This flagged some minor issues and places where destructuring could be used. These were the same errors seen with the ^15.0.0 version:

<Link> anchors invalid:

The <Link> elements in client/app/bundles/comments/layout/Layout.jsx produced ESLint errors with the new config:

The href attribute is required on an anchor. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid

Rules to fix this are documented, but the examples are in JSON and I couldn't convert or figure out a YAML version which would work in eslintrc.yml

I tried all of the following with no success, including vanilla JSON > YAML conversions:

jsx-a11y/anchor-is-valid:
  - error
  - components:
    - Link
    specialLink:
    - hrefLeft
    - hrefRight
    aspects:
    - noHref
    - invalidHref
    - preferButton

And:

jsx-a11y/anchor-is-valid: [error, {components: [Link], specialLink: [hrefLeft, hrefRight], aspects: [noHref, invalidHref, preferButton]}]

I finally turned it off: jsx-a11y/anchor-is-valid: off. Maybe someone better at YAML can fix this.

Changed an <a> link to a <button> with styling:

A link is used instead of a button in CommentBox.jsx, which gives the ESLint warning:

Anchor used as a button. Anchors are primarily expected to navigate. Use the button element instead  jsx-a11y/anchor-is-valid

I replaced the link with a button and styled it to look like the other links.

Newlines after parenthesis:

There were 18 places where newlines after parenthesis were causing ESLint errors, for example in commentsStore.js:17.

It was tough to figure out what the expected behavior should / would be from the docs. These errors were thrown with the eslintrc.yml default value of "function-paren-newline": ["error", "multiline"], but changing "multiline" to "consistent" took care of it.


This change is Reviewable

@justin808
Copy link
Member

@reconstructions Looks great so far. @alexfedoseev will take a peek.


Review status: 0 of 10 files reviewed at latest revision, 1 unresolved discussion.


client/package.json, line 108 at r1 (raw file):

    "chai-immutable": "^1.6.0",
    "eslint": "^4.11.0",
    "eslint-config-shakacode": "^16.0.0-rc.3",

we can maybe


Comments from Reviewable

@reconstructions
Copy link
Contributor Author

Thanks @justin808. Travis finished building but froze in test failures (tests were green locally), and probably should be restarted. Let me know if it would be easier for me to pull again instead.


Comments from Reviewable

@alex35mil
Copy link
Member

:lgtm:


Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.


client/.eslintrc.yml, line 20 at r1 (raw file):

rules:
  import/extensions: [2, { js: "never", jsx: "never" }]
  function-paren-newline: ["error", "consistent"]

Just FYI, we don't see this issue on fng.


client/package.json, line 108 at r1 (raw file):

Previously, justin808 (Justin Gordon) wrote…

we can maybe

FYI Published 16.0.0 (no changes from RC).


Comments from Reviewable

@reconstructions
Copy link
Contributor Author

client/.eslintrc.yml, line 20 at r1 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

Just FYI, we don't see this issue on fng.

I bumped the packaged to 16.0.0, and was able to --fix the newline errors (having cleaned up the unfixable ones before). ESLint fixed them by putting everything on one (long) line. Are their line breaks in FNG? Or maybe they got --fixed and are long? I will update this PR with the 16.0.0 package to see if that fixes the build. Let me know if you would prefer the function-paren-newline consistent config or --fixed long lines.


Comments from Reviewable

@reconstructions reconstructions force-pushed the eslint_update_revisions branch 2 times, most recently from aebb89a to 9c877c2 Compare November 30, 2017 22:11
@reconstructions
Copy link
Contributor Author

client/package.json, line 108 at r1 (raw file):

Previously, alexfedoseev (Alex Fedoseev) wrote…

FYI Published 16.0.0 (no changes from RC).

I bumped the version to 16.0.0, thx


Comments from Reviewable

@justin808 justin808 merged commit 8ca0a4b into shakacode:master Dec 4, 2017
@justin808
Copy link
Member

Thanks @reconstructions!

@reconstructions reconstructions deleted the eslint_update_revisions branch December 7, 2017 02:01
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 20, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 20, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 21, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 21, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 21, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 22, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 22, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 22, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 22, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 23, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 23, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 23, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 23, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 24, 2017
…hakacode#416

  * Yarn rolled back from "^1.0.0" to "0.22.0".
  * Pushes to Heroku, ActionCable working.
  * All tests and linting pass.
  * Non-breaking gems and js packages upgraded.
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 24, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Dec 24, 2017
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Jan 6, 2018
reconstructions added a commit to reconstructions/react-webpack-rails-tutorial that referenced this pull request Jan 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants