feat(eslint-plugin): Add rule triple-slash-reference

[jessetrinity]

Adding rule for corresponding tslint rule no-reference-import.

Should disallow doing both

/// <reference types="foo" />
//and
import * as foo from "foo"

in the same file.

Some thoughts:

1. I wasn't sure if it would be better to gate the rule checking behind the presence of the triple slash directive - input there would be appreciated.
2. How does one determine what the recommendation level / severity of a rule should be?
3. I probably need more test cases - I put in the obvious ones, but I could be more familiar with the module system.

[bradzacher]

How does one determine what the recommendation level / severity of a rule should be?

Set it to false (i.e. not recommended).
We cannot make changes to the recommended set without releasing a breaking change.

---

What are the situations that you might want triple slash reference imports?
I'm just wondering what the use cases are for this rule when compared to no-triple-slash-reference

[uniqueiniquity]

@bradzacher You'd want triple-slash references if you're writing an app without using modules but still want to enforce a dependency order between your source files. The TypeScript codebase is actually written like this - the whole thing is written with merged namespaces and triple-slash references that compile to a single output file.

Whoops, I was thinking about the other kind of triple slash reference. My bad.

[uniqueiniquity]

@bradzacher Let's try this again.
This rule addresses a different kind of triple-slash-reference than no-triple-slash-reference.
I think the use case here would be if you wanted access to types from a node_modules folder, but didn't want to make your file a module.

[JamesHenry]

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]

Codecov Report

Merging #625 into master will decrease coverage by 0.02%.
The diff coverage is 90.32%.

@@            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

Impacted Files	Coverage Δ
...lint-plugin/src/rules/no-triple-slash-reference.ts	100% <ø> (ø)	⬆️
packages/eslint-plugin/src/rules/index.ts	100% <100%> (ø)	⬆️
.../eslint-plugin/src/rules/triple-slash-reference.ts	90% <90%> (ø)

[bradzacher]

sorry If I'm being a little slow here...
It looks like this rule builds off of same logic as no-triple-slash-reference?

no-triple-slash-reference:

typescript-eslint/packages/eslint-plugin/src/rules/no-triple-slash-reference.ts

Lines 23 to 37 in aa206c4

	Program(program): void {
	const commentsBefore = sourceCode.getCommentsBefore(program);
	commentsBefore.forEach(comment => {
	if (comment.type !== 'Line') {
	return;
	}
	if (referenceRegExp.test(comment.value)) {
	context.report({
	node: comment,
	messageId: 'tripleSlashReference',
	});
	}
	});
	},

no-reference-import:

typescript-eslint/packages/eslint-plugin/src/rules/no-reference-import.ts

Lines 60 to 75 in bc63165

	Program(node) {
	programNode = node;
	const referenceRegExp = /^\/\s*<reference\s*types="(.*)"/;
	const commentsBefore = sourceCode.getCommentsBefore(programNode);
	commentsBefore.forEach(comment => {
	if (comment.type !== 'Line') {
	return;
	}
	const referenceResult = referenceRegExp.exec(comment.value);
	if (referenceResult && referenceResult[1]) {
	references.push({ comment, importName: referenceResult[1] });
	}
	});
	},

Couldn't the new logic be placed inside no-triple-slash-reference as an option instead of creating a new rule with duplicate logic?

[uniqueiniquity]

@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 ///<reference path=... /> and another option for what behavior you want for ///<reference types=... />. It's certainly feasible to combine those, though, especially if there's any perf advantages that you're aware of (beyond not iterating through the comments at the top of the file twice, of course).

[jessetrinity]

@bradzacher in addition to what @uniqueiniquity said, this rule does not strictly ban /// <reference types="" /> directives like the other rule does, but rather prevents you from using them if you also use an import declaration for that package (I realize my documentation for this was misleading - will change).

A unified rule would need to specify severity levels for each species of reference directive, something like no-duplicate and banned for each of /// <reference path="" /> and /// <reference types="" /> (we also have /// <reference lib="" />).

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 :)

[bradzacher]

AHH! That's what I missed: /// <reference ****types****=..... SORRY.

---

Spitballing here.

What do you guys think about this direction:

• rename this new rule to triple-slash-reference
  • we're kinda trying to remove no-* rule names.
• add an options object (see below)
• deprecate no-triple-slash-reference naming this new rule.

type Options = [
    {
        lib: 'never' | 'allow' | 'allow-when-not-imported',
        path: 'never' | 'allow',
        types: 'never' | 'allow',
    }
];

[uniqueiniquity]

Seems reasonable to me. I assume you meant lib, path, and types, but let me know if I'm missing something.

Thoughts, @jessetrinity?

[jessetrinity]

Sure.

lib: 'never' | 'allow' | 'allow-when-not-imported',

Is there already a pattern for rule option names? Something like 'banned' | 'allow' | 'prefer-import' would be more obvious to me.

[bradzacher]

nah there's no guidelines, it's more of a "whatever makes sense" thing.
Maybe 'ban' instead of 'banned'. Just to keep the tense consistent with 'allow'.
everything else looks good! thanks!

[jessetrinity]

Maybe I'm being too conservative, but I did not add the prefer-import option to /// <reference path="" /> or /// <reference lib="" /> directives as they were not covered in existing rules.

[bradzacher]

one thing, code otherwise looks good to me!
great work, thanks for your contribution!

[JamesHenry]

@bradzacher it seems to be failing on documentation validation, any tips for @jessetrinity on how to get the build to pass?

[bradzacher]

glancing at the build output
https://dev.azure.com/typescript-eslint/TypeScript%20ESLint/_build/results?buildId=1344

Found rule no-triple-slash-reference in table, but it doesn't exist in the plugin.
Just need to delete the no-triple-slash-reference line from the table in the readme (the rule has been deprecated, so it shouldn't be advertised in the root readme any more)

stale

[milesj]

This was super confusing. The title says its no-reference-import, while the old tslint was no-triple-slash-reference, but now its triple-slash-reference.

[jessetrinity]

This was super confusing. The title says its no-reference-import, while the old tslint was no-triple-slash-reference, but now its triple-slash-reference.

Sorry about that. There is a separate tslint no-reference-import rule this was meant to mirror. it ended up absorbing no-triple-slash-reference so we tried a more generic name. I think there is a desire to move away from the "no-foo" naming convention.

[bradzacher]

sorry! I've updated the release notes / pr title.

@jessetrinity is correct. The standard we want to move toward is not having no-* rules unless it makes literally no sense to ever use the *.
Additionally tslint had two rules which did very similar things. Because we're doing a clean implementation, it made sense to merge the rules together whilst making it more flexible by allowing the inverse.

The old rule no-triple-slash-reference still exists in v1 of the plugin, but will be removed in v2.

[milesj]

Makes sense, thanks for the quick turn around!