-
Notifications
You must be signed in to change notification settings - Fork 221
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 nocycle linting #1700
Conversation
src/server/index.jsx
Outdated
@@ -38,6 +39,8 @@ class LoggerStream { | |||
|
|||
const server = express(); | |||
|
|||
console.log(routes, articleRegexPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log left in place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
Hi @rossgaskell thanks, this looks good, just for posterity and the next person scratching their head over any similar warnings can you briefly summarise why "Move route regex to separate file" fixed the linter error? |
No problem, I'll add my investigation notes from the issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Nice Refactor.
Looks good to me.No more linting errors seen when |
Resolves #1622
Overall change: Re-enable no-cycle linting errors
Code changes:
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.