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

[New] no-cycle: add option to allow cycle via dynamic import #2387

Merged
merged 1 commit into from May 1, 2022

Conversation

GerkinDev
Copy link
Contributor

@GerkinDev GerkinDev commented Feb 21, 2022

Hi,

as discussed in #2265, I added a new option, allowUnsafeDynamicCyclicDependency. I've tested all cases I could.

Unfortunately, I did not managed to write tests using the TS_OLD parser. I don't know if this is important.

Feedbacks are welcome ! Have a nice day

Fixes #2265.

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Feb 21, 2022

Seeing the CI job https://app.travis-ci.com/github/import-js/eslint-plugin-import/builds/246801187, it seems that eslint<4 does not parse dynamic imports in a way that allows cyclic dependencies detection with dynamic imports, Yet, it passes for later versions. I don't really know what to do. Should I remove this test ?

@codecov
Copy link

codecov bot commented Feb 21, 2022

Codecov Report

Merging #2387 (0fe7fc1) into main (be30a34) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Current head 0fe7fc1 differs from pull request most recent head b2f6ac8. Consider uploading reports for the commit b2f6ac8 to get more accurate results

@@            Coverage Diff             @@
##             main    #2387      +/-   ##
==========================================
- Coverage   95.12%   95.04%   -0.08%     
==========================================
  Files          66       66              
  Lines        2749     2725      -24     
  Branches      923      918       -5     
==========================================
- Hits         2615     2590      -25     
- Misses        134      135       +1     
Impacted Files Coverage Δ
src/ExportMap.js 100.00% <ø> (ø)
src/rules/no-cycle.js 97.91% <100.00%> (+0.24%) ⬆️
src/rules/newline-after-import.js 95.31% <0.00%> (-0.85%) ⬇️
src/rules/no-unused-modules.js 97.43% <0.00%> (-0.24%) ⬇️
src/rules/order.js 99.06% <0.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be30a34...b2f6ac8. Read the comment docs.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2022

@GerkinDev no, but it can be skipped on older eslint versions.

Copy link
Member

@ljharb ljharb left a comment

I'd like to make sure we also test the case where there's a cycle that's accessed via dynamic import - that even with this option, the static cycle is warned on.

docs/rules/no-cycle.md Show resolved Hide resolved
@GerkinDev GerkinDev force-pushed the feature/allow-dynamic-cycle-2265 branch from 77cf864 to 0fe7fc1 Compare Mar 7, 2022
@ljharb ljharb force-pushed the feature/allow-dynamic-cycle-2265 branch from 0fe7fc1 to b2f6ac8 Compare Apr 30, 2022
@ljharb
Copy link
Member

ljharb commented Apr 30, 2022

I've rebased this, and ensured no tests were removed. If everything passes, this should be good to go.

ljharb
ljharb approved these changes May 1, 2022
@ljharb ljharb changed the title Feature: allow dynamic cycle (#2265) [New] no-cycle: add option to allow cycle via dynamic import May 1, 2022
@ljharb ljharb merged commit b2f6ac8 into import-js:main May 1, 2022
143 checks passed
@Anber
Copy link

Anber commented May 9, 2022

It looks like the changes weren't published https://unpkg.com/eslint-plugin-import@2.26.0/lib/rules/no-cycle.js

@ljharb
Copy link
Member

ljharb commented May 9, 2022

Yet. They were only merged 8 days ago. You can click on the merging commit hash to see that there’s no release tags attached to it.

@Anber
Copy link

Anber commented May 9, 2022

However, it is mentioned in the changelog as released in 2.26.0 https://github.com/import-js/eslint-plugin-import/blob/main/CHANGELOG.md#2260---2022-04-05

@ljharb
Copy link
Member

ljharb commented May 9, 2022

you're right, thanks, i'll fix that.

@ljharb
Copy link
Member

ljharb commented May 9, 2022

Note that the v2.26.0 changelog doesn't have that mistake: https://github.com/import-js/eslint-plugin-import/blob/v2.26.0/CHANGELOG.md

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue May 9, 2022
@GerkinDev GerkinDev deleted the feature/allow-dynamic-cycle-2265 branch Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants