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

add eslint-import rule no-cycle #656

Merged
merged 2 commits into from
Nov 30, 2023
Merged

add eslint-import rule no-cycle #656

merged 2 commits into from
Nov 30, 2023

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Nov 27, 2023

Description

Added the eslint-import plugin and activated the no-cycle rule, so we'll catch any accidental circular dependencies.

Motivation and Context

I saw the comment in glyph.js regarding the circumvention of a possible circular dependency with tables/glyf.js and wanted to make sure we catch similar situations in the future.

How Has This Been Tested?

npm run lint

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@Connum Connum requested review from ILOVEPIE and yne November 27, 2023 20:45
@Connum Connum added the dev Changes revolving merely around dev-related code like testing, build process, etc. label Nov 27, 2023
Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@Connum Connum merged commit 7f1480f into master Nov 30, 2023
1 check passed
@Connum Connum deleted the dev/eslint-import-no-cycle branch November 30, 2023 21:15
Copy link
Contributor

@yne yne left a comment

Choose a reason for hiding this comment

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

isn't it already supposed to be detected at ci level (when building or running the tests ?)

@Connum
Copy link
Contributor Author

Connum commented Nov 30, 2023

I think I tested it by producing a circular dependency in the glyf file and esbuild didn't complain about it iirc. But it's still an antipattern.

@yne
Copy link
Contributor

yne commented Apr 21, 2024

I added a import {parse} from './opentype.mjs'; in font.mjs (and added a fake if(parse)parse; in a font function)

but npm run lint did not reported it.

Did I missunderstood it usage ? Or did it simply never worked ?

@Connum
Copy link
Contributor Author

Connum commented Apr 21, 2024

It must have worked at one point, or I wouldn't have made the PR :) But maybe some changes lead to it not working anymore, as we don't have any tests for the linting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Changes revolving merely around dev-related code like testing, build process, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants