Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Support outlawing /// <reference> style imports. #1139

Merged
merged 1 commit into from
Apr 20, 2016

Conversation

mprobst
Copy link
Contributor

@mprobst mprobst commented Apr 19, 2016

The reasoning is that these are superseded by ES6 style imports, and the syntax
overall isn't too pleasant.

@mprobst mprobst force-pushed the no-reference branch 2 times, most recently from 7776fd9 to 5999d7a Compare April 19, 2016 01:02

class NoReferenceWalker extends Lint.RuleWalker {
public visitSourceFile(node: ts.SourceFile) {
for (let ref of node.referencedFiles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

node.referencedFiles only refers to <reference path= statements, which is good

@jkillian
Copy link
Contributor

I like this rule too! This theme of rules that encourages using modern-TS is a good use-case for TSLint in my opinion.

Couple notes: TS is adding a <reference types=...> directive. We might want to allow that - I haven't read enough into its use case yet to say for certain though. There's also <reference no-default-lib="true"/> which we also probably want to allow (and your code does).

With all that in mind, I think we should change the name of this rule slightly, perhaps to no-reference-path or no-path-reference or something similar?

@@ -90,6 +90,7 @@
"../src/rules/noInternalModuleRule.ts",
"../src/rules/noInvalidThisRule.ts",
"../src/rules/noNullKeywordRule.ts",
"../src/rules/noReferenceRule.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

src/tsconfig.json should also have a similar change, running grunt should automatically update both files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mprobst
Copy link
Contributor Author

mprobst commented Apr 19, 2016

What about making this no-reference, and then adding disallow-types if it becomes a thing? It makes sense to me to make anything that's reference related in one option, as much as that's possible.

So "no-reference": true for now, and then later if we need to support it "no-reference": [true, "disallow-types"].

The reasoning is that these are superseded by ES6 style imports, and the syntax
overall isn't too pleasant.
@jkillian
Copy link
Contributor

👍 Agreed, keeping it as no-reference seems fine, and various options (either negative or positive) can be added as needed later.

@mprobst
Copy link
Contributor Author

mprobst commented Apr 19, 2016

Cool. I think this is good to go then, unless I'm missing something.

@jkillian jkillian merged commit 3c80d83 into palantir:master Apr 20, 2016
@jkillian
Copy link
Contributor

Agreed - thanks for the PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants