[eslint-plugin-packlets] Initial implementation#2256
Conversation
octogonz
requested review from
D4N14L,
apostolisms,
halfnibble,
iclanton,
patmill and
sachinjoseph
as code owners
sachinjoseph
left a comment
sachinjoseph
left a comment
There was a problem hiding this comment.
LGTM. Left a few clarification comments.
| return undefined; | ||
| } | ||
|
|
||
| public static detectCircularImport( |
There was a problem hiding this comment.
I assume we want to return only the first cycle if there are multiple cycles?
There was a problem hiding this comment.
I should add some docs to clarify this. The way it works:
- Cycles are only detected by looking at
index.tsfiles. This works because we require consumers to import those files. - Each
importstatement is analyzed separately, and the analysis stops when the first cycle is found. - We then sort the list of affected packlets alphabetically, and only report the error for the
index.tsfile of the first packlet in the list. This avoids redundantly reporting it for each packlet along the cycle.
| return; | ||
| } | ||
|
|
||
| if (!(modulePath.startsWith('.') || modulePath.startsWith('..'))) { |
There was a problem hiding this comment.
Can we import from a file in the same folder like import from 'filename.ts' without having to do ./filename.ts?
There was a problem hiding this comment.
We can't. The module resolver would interpret filename.ts to be an NPM package with that name.
| // Okay | ||
| import { MainReport } from '../packlets/reports'; | ||
|
|
||
| // Error: The import statement does not use the packlet's entry point (@rushstack/packlets/mechanics) |
There was a problem hiding this comment.
Please allow this. This is strictly superior to the directory import in terms of resolution accuracy and performance in TypeScript, Webpack, etc.
There was a problem hiding this comment.
Could you provide some details? If this is a real concern (?), then it seems better to optimize our tooling than to ask developers to write their code in an awkward way.
There was a problem hiding this comment.
@dmichon-msft I'm interested in this question, but I'm trying to unblock a couple teams who are waiting for these lint rules. So if you feel strongly about it, let's sort it out in a separate GitHub issue.
CC @iclanton
There was a problem hiding this comment.
Expecting people to always say import { ... } from 'package-name/lib/index'; is awkward and will break SPFx development scenarios.
This isn't an optimization that we should expect developers to have to make because it produces objectively worse code for the sake of making the compiler slightly faster. If these filesystem accesses are a real concern, we should try to improve our caching and maybe explore filling a filesystem cache in a background thread.
There was a problem hiding this comment.
I'm fine with not making it the default. I'm not so happy about forbidding it. If we were actually writing code directly for browsers, we'd be expected to include the final step of doing import { MainReport } from '../packlets/reports/index.js', but unfortunately TypeScript doesn't allow us to be that specific.
| const referencingFilePath: string = refFile.file; | ||
|
|
||
| if (Path.isUnder(referencingFilePath, packletsFolderPath)) { | ||
| const referencingRelativePath: string = path.relative(packletsFolderPath, referencingFilePath); |
There was a problem hiding this comment.
All file paths within TypeScript are normalized to use / as the directory separator. Recommend using path.posix.relative and, if necessary, starting at the first / if there are problems with the drive letter on Windows.
4 participants
Fixes #2254