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

Allow up to 10 imports/dependencies #20

Merged
merged 1 commit into from
Dec 29, 2023
Merged

Conversation

screendriver
Copy link
Member

import type a from './a.js';

Should not be count as a dependency in my opinion. Often I need a lot of types in one module, especially when working with dependency injection, but not as many "real" runtime imports. Therefore there should be no limit in allowing type-only imports.

@screendriver screendriver self-assigned this Dec 25, 2023
@lo1tuma
Copy link
Member

lo1tuma commented Dec 25, 2023

🤔 I’m not sure about this one. Do you have a concrete example where the amount of type imports does NOT have a negative impact on the readability of the module and thus on the complexity of the business object?
A type import usually means that there is some kind of interaction with a different interface. Having many of those means the module might interact with too many other interfaces and might be too tightly coupled to other modules. See also https://blog.devgenius.io/code-smell-94-too-many-imports-e8f44986db2.

Maybe 8 is too restrictive when working with typescript, but I think allowing unlimited amount of type imports is bad for readability and complexity. It would be nice if we could configure two separate limits, one for regular imports and one for type-imports.

@screendriver
Copy link
Member Author

screendriver commented Dec 25, 2023

Do you have a concrete example where the amount of type imports does NOT have a negative impact on the readability of the module and thus on the complexity of the business object?

Right now I'm preparing a pull request for pr-log where I need one import more in cli.ts. Right now it has 3 runtime imports and 5 type imports. Unfortunately import/max-dependencies is not able to differentiate between runtime and type-only imports 😕 My wish would be to set two dedicated limits for both.

Maybe 8 is too restrictive when working with typescript

Yes. But which magic number should we set then? 10? 12?

It would be nice if we could configure two separate limits, one for regular imports and one for type-imports.

💯 as I also wrote above.

@lo1tuma
Copy link
Member

lo1tuma commented Dec 29, 2023

Yes. But which magic number should we set then? 10? 12?

I don’t think it should be an arbitrary magical number. Ideally we could rely on neuroscience. How many import statements can be easily read and keep in the current cognitive context while reading the code of the remaining file?
For example 50 would be clearly to many. The question is how many would be the maximum that helps to keep the code small and readable? I think I would be ok the raise the limit a bit. So maybe 10?

Regarding the pr-log example, I actually do think that this is a sign that there is another layer missing. The cli runner should contain any complicated logic and should only translate the commands and options to the necessary function calls on the pr-log main business object, which currently doesn’t exist.

@screendriver screendriver changed the title Ignore type imports in max-dependencies rules Allow up to 10 imports/dependencies Dec 29, 2023
@screendriver
Copy link
Member Author

The question is how many would be the maximum that helps to keep the code small and readable?

I don't know. Personally I look never at imports and immediately jump to the very first lines of the implementation.

I think I would be ok the raise the limit a bit. So maybe 10?

I changed it to 10 now.

The cli runner should contain any complicated logic and should only translate the commands and options to the necessary function calls on the pr-log main business object, which currently doesn’t exist.

Then maybe we should work on that after my changes regarding --unreleased 🙂

@screendriver screendriver merged commit 12d1afb into main Dec 29, 2023
2 checks passed
@screendriver screendriver deleted the ignore-type-imports branch December 30, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants