feat(linter): add allowedExternalImports option to boundaries rule #13891
Conversation
yjaaidi
commented
Dec 17, 2022
yjaaidi
commented
- chore(linter): add empty tests
- chore(linter): add wip tests for allowedExternalImports
- feat(linter): add allowedExternalImports option
eslint's enforce-module-boundaries' allowedExternalImports option will only allow the project to import npm packages matching the string or wildcard Closes nrwl#12870
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
yjaaidi
changed the title
|
|
||
| /* ... then check if there is a whitelist and if there is a match in the whitelist. */ | ||
| return ( | ||
| allowedExternalImports != null && |
There was a problem hiding this comment.
This comparison by value got me confused for a bit. Can we replace it with just allowedExternalImports && ...?
Perhaps it would be cleaner to use elvis + every to be in sync with bannedExeternalImports:
return allowedExternalImports?.every(importDefinition =>
!parseImportWildcards(importDefinition).test(packageName)
);There was a problem hiding this comment.
This comparison by value got me confused for a bit. Can we replace it with just allowedExternalImports && ...?
I just like to avoid the boolean coercion and make it explicit that I am not mistakenly (thinking that [] == false just like '' == false) trying to check that the array is not empty.
That said, I'm totally alright with the coercing instead and going with allowedExternalImports && ....
Now, concerning optional chaining + every, the problem is that it doesn't make it very explicit that we are expecting allowedExternalImports = [] to return true... which actually happens because [].every(() => true) returns true.
That said, the current implementation !allowedExternalImports.some(...) doesn't make it more expclit 😅.
Anyway, there is a test that makes it clear we are expecting allowedExternalImports = [] to make any external package import fail so I am ok with any implementation.
We can switch to allowedExternalImports?.every(...) (which ends up being my favorite).
|
@meeroslav I made some changes and added explanation in comments. Let me know what you think |
meeroslav
changed the title
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
