Skip to content
This repository has been archived by the owner on Nov 30, 2020. It is now read-only.

Feature/45s update eslint plugin import #61

Closed
wants to merge 4 commits into from

Conversation

frycz
Copy link
Member

@frycz frycz commented Feb 17, 2020

Closes #45

@frycz frycz requested a review from TkDodo February 17, 2020 15:42
@@ -0,0 +1,6 @@
---
issue: https://github.com/pmedianetwork/eslint-config/issues/45
type: minor
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also have to actually set the rule:

"import/no-cycle": "error"

It is not set at all at the moment. Currently, we only have it in insights, and there, we set it to "warn".

IF we do this, it's most likely a breaking change. Do we want that @danielbartsch
I am also not sure how this will integrate with typescript ... But we could just say that repos that don't want it that strict need to "downgrade" the rule "warn" for themselves .. ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a good rule to have.
If one some of the projects have issues with it, as last resort they can disable it with "import/no-cycle": 0. That could be a good opportunity to check the health of the codebase though.

Copy link
Contributor

Choose a reason for hiding this comment

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

since this import-js/eslint-plugin-import#1494 is merged, I'd do it, even tho yes, it is a breaking change.

But since we have 2 other PRs merged, I'd do this after the release.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, I'll make a minor release and then we can maybe do this as a breaking release, maybe together with #39

@TkDodo
Copy link
Contributor

TkDodo commented Jul 29, 2020

let's also revisit this once we have the monorepo

@TkDodo TkDodo closed this Jul 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update eslint-plugin-import (no-cycle)
4 participants