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

Re-enable no-cycle linter warnings #1622

Closed
1 task done
jamesdonoh opened this issue Apr 23, 2019 · 1 comment · Fixed by #1700
Closed
1 task done

Re-enable no-cycle linter warnings #1622

jamesdonoh opened this issue Apr 23, 2019 · 1 comment · Fixed by #1700
Assignees
Labels
bug Something isn't working
Projects

Comments

@jamesdonoh
Copy link
Contributor

jamesdonoh commented Apr 23, 2019

Describe the bug
In #1597 we updated our dependencies but had to add to temporarily disable import/no-cycle in .eslintrc.js.

This is because with that warning enabled we now get 10 errors of the form:

/path/to/simorgh/src/app/containers/Article/index.jsx
  10:1  error  Dependency cycle via ../Text:6=>../Paragraph:2=>../InlineLink:5=>../../routes:10  import/no-cycle

We should re-enable the linter rule and fix the errors.

To Reproduce
Steps to reproduce the behavior:

  1. Remove import/no-cycle from .eslintlrc
  2. Run npm run test:lint

Expected behavior
No linter errors should be shown.

Screenshots
NA

Additional context
Add any other context about the problem here.

  • Initially labelled with "bug"
@jamesdonoh jamesdonoh added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. simorgh-core-stream labels Apr 23, 2019
@jamesdonoh jamesdonoh added this to To do in Simorgh via automation Apr 23, 2019
@jamesdonoh jamesdonoh added the bug Something isn't working label Apr 23, 2019
@chris-hinds chris-hinds removed the Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. label Apr 26, 2019
@chris-hinds chris-hinds moved this from To do to Ready for dev in Simorgh Apr 26, 2019
@rossgaskell rossgaskell moved this from Ready for dev to Issue in Progress in Simorgh May 8, 2019
@rossgaskell rossgaskell self-assigned this May 8, 2019
@rossgaskell
Copy link
Contributor

Investigation

The errors started appearing due to the bump in eslint import plugin version. There was a bug with the babel-eslint parser not reporting the errors. The errors reported already existed before the dependency update in #1597, but were not reported.

import-js/eslint-plugin-import#1218
import-js/eslint-plugin-import#1166

The warnings are a false negative. Whilst there is a circular dependancy due to the internal link requiring the routes regex to match against, this only imports a constant and therefore isn't a true circular dependency.

However to improve readability, the regex paths have been moved to separate file that the routes and internal link component can share without eslint flagging a circular dependency.

Simorgh automation moved this from Issue in Progress to Done May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants