-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): Add rule triple-slash-reference #625
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
feat(eslint-plugin): Add rule triple-slash-reference #625
Conversation
Set it to false (i.e. not recommended). What are the situations that you might want triple slash reference imports? |
Whoops, I was thinking about the other kind of triple slash reference. My bad. |
@bradzacher Let's try this again. |
A good case is needing to directly tie a spec/test file to the types for the test runner you are using. The majority of test runners just create globals for their utils, and, particularly in a monorepo, you may have multiple different test runners doing this, making it difficult to have the right ambient types associated with the right files. I believe I also remember Alex Eagle mentioning this is also a consideration for JS test runners in a Bazel context, where Bazel has to know all the inputs explicitly in order to function correctly. |
Codecov Report
@@ Coverage Diff @@
## master #625 +/- ##
==========================================
- Coverage 94.36% 94.33% -0.03%
==========================================
Files 110 111 +1
Lines 4560 4591 +31
Branches 1258 1267 +9
==========================================
+ Hits 4303 4331 +28
Misses 149 149
- Partials 108 111 +3
|
sorry If I'm being a little slow here...
typescript-eslint/packages/eslint-plugin/src/rules/no-triple-slash-reference.ts Lines 23 to 37 in aa206c4
Couldn't the new logic be placed inside |
@bradzacher Sure, we could combine the two; they're separate at least because they were separate in TSLint. However, note that it's not as simple as an option to toggle between the two implementations; in a unified rule, you'd have to have an option for whether you wanted to allow or disallow |
@bradzacher in addition to what @uniqueiniquity said, this rule does not strictly ban A unified rule would need to specify severity levels for each species of reference directive, something like I don't know the logic behind tslint's decision to have a rule banning one and de-duping the other. I have no problem combining them but I think we were converting rules in the spirit of being consistent with tslint. EDIT: re-read the comment, I guess I didn't add anything new to what @uniqueiniquity said, so yes, I agree :) |
AHH! That's what I missed: Spitballing here. What do you guys think about this direction:
type Options = [
{
lib: 'never' | 'allow' | 'allow-when-not-imported',
path: 'never' | 'allow',
types: 'never' | 'allow',
}
]; |
Seems reasonable to me. I assume you meant Thoughts, @jessetrinity? |
Sure.
Is there already a pattern for rule option names? Something like |
nah there's no guidelines, it's more of a "whatever makes sense" thing. |
bc63165
to
ecc65e1
Compare
Maybe I'm being too conservative, but I did not add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing, code otherwise looks good to me!
great work, thanks for your contribution!
@bradzacher it seems to be failing on documentation validation, any tips for @jessetrinity on how to get the build to pass? |
glancing at the build output
|
This was super confusing. The title says its |
Sorry about that. There is a separate tslint |
sorry! I've updated the release notes / pr title. @jessetrinity is correct. The standard we want to move toward is not having The old rule |
Makes sense, thanks for the quick turn around! |
Adding rule for corresponding tslint rule no-reference-import.
Should disallow doing both
in the same file.
Some thoughts: