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

import/no-cycle should not trigger for Flow imports #1343

Closed
murrayju opened this issue Apr 23, 2019 · 10 comments · Fixed by #1494
Closed

import/no-cycle should not trigger for Flow imports #1343

murrayju opened this issue Apr 23, 2019 · 10 comments · Fixed by #1494

Comments

@murrayju
Copy link

This is a regression of #1098 (fix in PR #1126) that was re-introduced in v2.17.0.

v2.16.0 works as expected. v2.17.2 still has this regression.

@jonathanrdelgado
Copy link

Not sure if it is on the radar of this project, but it is also worth mentioning there is a similar issue with Typescript.

@ljharb
Copy link
Member

ljharb commented May 20, 2019

@jonathanrdelgado it is, if an issue is filed - please file one if there isn’t one already.

@ljharb ljharb added the flow label May 20, 2019
@SebastienGllmt
Copy link

SebastienGllmt commented Jun 23, 2019

Has anybody had time to look into this? 2.17.3 still has this regression 🙏

@SebastienGllmt
Copy link

SebastienGllmt commented Jul 20, 2019

Hey folks. I looked into this and it turns out the reason for the regression is that the original fix was subsequently removed by this pr. You can tell that #1218 is the cause of the issue because commit 70a59fe is the first commit that introduces this regression

if you look at the current state of the file, it hasn't changed since this break
https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-cycle.js#L34

@ljharb
Copy link
Member

ljharb commented Jul 20, 2019

Thanks, seems like a simple PR to restore it, but with a test case to prevent it from being removed again.

@SebastienGllmt
Copy link

I don't really have context for why this was removed so I may not be the best person for this. I tried to write a test case to reproduce the error we get on our codebase but to my surprise, no error occurs. It could be that I'm not using the tests properly but it could also be that this error only is reproducible with a certain kind of setup.

@dominicfraser
Copy link

https://github.com/benmosher/eslint-plugin-import/pull/1494 was opened to fix this on 5th Oct, but as yet has not been approved.
cc: @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 24, 2019

Yes, thanks, I'm aware. It's been on my list.

@dominicfraser
Copy link

Great, thanks 😀

@dominicfraser
Copy link

Thank you for this :D :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants