Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Add feature to import-name rule ignoring node_modules and etc ( #541 ) #569

Merged
merged 21 commits into from
Oct 24, 2018

Conversation

lonyele
Copy link
Contributor

@lonyele lonyele commented Oct 17, 2018

Hi, It's a follow-up PR: fixes #541

I try not to make any conflict with current structure thus I make getting more arguments for below features
First: same as before (boolean)
Second: same as before ({ [indes: tring] : string })
Third: get the array of module names that user want to ignore not replacing (string[])
Fourth: currently 'ignoreExternalModule' : true by default. It prevents from tslint outputting error by checking the path of imported module at runtime and check If it is an external module(In this case any things from ./node_modules/*

I'm a junior developer, technically I'm not even a junior developer since I haven't had a full time job yet. So please check If I've done right.

@msftclas
Copy link

msftclas commented Oct 17, 2018

CLA assistant check
All CLA requirements met.

@lonyele
Copy link
Contributor Author

lonyele commented Oct 21, 2018

I think only a few people will actually use this rule to be this extend, but as I started adding features ended up like this.

Why taking more arguments? It looks too much.
-> I didn't want any conflict with existing structures, also more features using only one existing argument doesn't look sufficient

About third argument
-> I make getting arrays of string because user possibly wants more than one module to be igrnored. I made to give two options at the same instead of getting either replacements or ignored modules for second argument(#541 (comment)) I think user wants some modules to be replaced and some modules to be ignored without replacement.

About fourth argument
-> this is the feature that I wanted... totally ignoring external modules(npm installed packages) because user doesn't have any control over this code. It checks module path at runtime if It is an external module or not.

By the way, I didn't update the readme file because of not knowing how to write version section. Here are my updates that may be used for updating readme doc.

If you want to just ignore some modules other than replacing with specific names, put arrays of module names in string to the third argument. such as
'import-name': [true, { 'underscore': '_' }, ["whatever-module", "to-ignore"]}

By default, 'import-name' rule will exclude npm installed packages that reside at node_modules folder, but you can make this config false by passing config object to the fourth argument.
'import-name': [true, { 'underscore': '_' }, ["whatever-module", "to-ignore"], { ignoreExternalModule: false }}

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for this @lonyele! The rule configuration is confusing but I don't think that's anything you can fix in this PR - seems like a pre-existing structural shenanigan.

Could you please document in the rule's metadata how the options work? After this PR goes in we can file a followup issue to make them less confusing.

tsconfig.json Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link

update the readme file

Don't worry about that 😊 I'll go through all the PRs before the next release and write that.

2. Without es7 support, Array.includes doesnt work. I changed it using filter function. (This is the reason why I added es7 support)  I think Array.includes() is not that 'absolutely needed' I just removed it.
1. no-any rule
--> change to unknown
2. using isObject TypeGuard
3. resolve type issues by temporarily making interfaces
@lonyele
Copy link
Contributor Author

lonyele commented Oct 23, 2018

After changing to 'unknown' type, I got the errors even if I type guard object property. I've looked through all the types I could see on typescript.d.ts, It is beyond my skill. I made interfaces temporarily to make things work, I think It needs some reviews.
My guess is that either typescript.d.ts doesn't reflect real usage in runtime or many more properties/attributes are added at runtime. (I could see something like ModuleResolutionCache, NonRelativeModuleNameResolutionCache)

--> naming of previous one can be confusing.
Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM. I'll file a followup issue for cleaning up the configuration options & interfaces.

@JoshuaKGoldberg
Copy link

Thanks for your persistence on this @lonyele! 🚀

@JoshuaKGoldberg JoshuaKGoldberg merged commit e076605 into microsoft:master Oct 24, 2018
@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0-beta0 milestone Nov 6, 2018
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.

Add an importNameRule option to ignore npm modules
3 participants