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

Deprecate no-use-before-declare rule for typescript 2.9+ #4695

Merged
merged 1 commit into from
May 5, 2019
Merged

Deprecate no-use-before-declare rule for typescript 2.9+ #4695

merged 1 commit into from
May 5, 2019

Conversation

waseemahmad31
Copy link
Contributor

PR checklist

Overview of change:

deprecate no-use-before-declare for TS 2.9+

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @waseemahmad31! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Super, thanks!

@JoshuaKGoldberg JoshuaKGoldberg merged commit 5a3a640 into palantir:master May 5, 2019
@karol-majewski
Copy link
Contributor

It seems like discouraging TSLint users from using no-use-before-declare might have been premature. There is a class of errors this rule helps catch and these errors are not detected by TypeScript itself.

Consider this example:

const consumer = () => {
  console.log(variable); // no-use-before-declare: "variable 'variable' used before declaration"
};

consumer();

const variable = 'foo';

This snippet represents perfectly valid TypeScript — no compile-time error will be issued. Yet, an attempt to run it will cause a runtime exception.

See TypeScript playground.

Thanks to @OliverJAsh for providing an example.

@JoshuaKGoldberg
Copy link
Contributor

@karol-majewski one difficulty of rules like no-use-before-declare is figuring out which nodes are truly used before declaration vs. ones that just seem to be used before declaration.

const consumer = () => {
    setTimeout(() => {
        console.log(variable);
    }, 1);
};
  
consumer();
  
const variable = 'foo';

The rule has been somewhat marked as deprecation for a while (#4666); if you'd like to see it un-deprecated or generally re-implemented please file a new issue.

@OliverJAsh
Copy link
Contributor

OliverJAsh commented May 5, 2019

The docs seem to suggest this rule was deprecated because it no longer has any use (since var is no longer used). Is that correct?

In addition to @karol-majewski's point about catching runtime errors: I would still like this rule for a code style preference of mine: enforcing single direction of code definitions—details at top, composition/usages below. E.g.

Without

const y = 1;

const foo = () => {
    x;
    y;
}

const x = 1;

With

const x = 1;
const y = 1;

const foo = () => {
    x;
    y;
}

Of course this is subjective—just pointing out that the rule was useful for enforcing this preference, for people/teams who like it.

@adidahiya adidahiya mentioned this pull request May 30, 2019
@Burgestrand
Copy link

Burgestrand commented Jun 11, 2019

I'm a new user of typescript, and received this warning. However, I believe it's unclear:

Please use the built-in compiler checks instead.

Does this mean I need to enable a compiler option in order to have the same guarantees as I would have had before this rule was deprecated?

I propose changing the message to either "This rule is no longer necessary and can be safely removed" or pointing the user (in this case me) to documentation for which compiler options are relevant.

However, to be fair, it seems to me the discussion here isn't quite over yet.

@bolatovumar
Copy link
Contributor

@Burgestrand I can't find an equivalent compiler option for this rule but it looks like you don't really need this rule if you don't use the "var" keyword. So, if you have the "no-var-keyword" rule enabled then this rule is not needed at all.

@Burgestrand
Copy link

@bolatovumar Great, thank you!

@karol-majewski
Copy link
Contributor

@bolatovumar None of the examples in this thread is using the var keyword. 🤔 Yet using no-use-before-declare would have helped catch errors for them.

@bolatovumar
Copy link
Contributor

@karol-majewski you get compiler warnings if you use a const or let variable before it's declared so you don't need this rule in that case.

@karol-majewski
Copy link
Contributor

@bolatovumar TypeScript doesn't issue any warnings for this piece of code and yet it breaks in runtime:

const consumer = () => {
  console.log(variable);
};

consumer();

const variable = 'foo';

See for yourself in TypeScript playground.

@bolatovumar
Copy link
Contributor

@karol-majewski right, I see. You might wanna file a separate issue as @JoshuaKGoldberg suggested above . I'm not actually sure why this rule is deprecated specifically for TS 2.9+ so I can't really add much more here.

@JoshuaKGoldberg
Copy link
Contributor

Posting again here: the rule seems to have some buggy behavior, such as the false positive mentioned in #2551. If you'd like the rule to be un-deprecated despite that (maybe the bug doesn't exist? maybe it's worth it to keep the rule?) please file a new issue so we can discuss there. PR responses aren't very search-discoverable.

Also, keep #4534 in mind: TSLint is in the process being deprecated.

Same goes for suggestions to improve the rule's docs or error messaging. I'm all for improving docs, but we should discuss them in a dedicated thread. It's hard to keep track of this one... 😞

@ikokostya
Copy link

ikokostya commented Jul 10, 2019

please file a new issue so we can discuss there

Come on. I created new issue #4789, but you closed it with blocked status without any discussion.

Also, keep #4534 in mind: TSLint is in the process being deprecated.

Yes, maybe TSLint team will stop deprecate existing rules? False positive result is not a reason for removing rule. By this logic any buggy rule can be removed.

Posting again here: the rule seems to have some buggy behavior, such as the false positive mentioned in #2551. If you'd like the rule to be un-deprecated despite that (maybe the bug doesn't exist? maybe it's worth it to keep the rule?

So, example from issue #2551 is not isolated and contains some project specific entities.

I tried to test code from replies:
#2551 (comment)
#2551 (comment)

There are no errors with no-use-before-declare rule and latest version of tslint.

@JoshuaKGoldberg
Copy link
Contributor

Come on. I created new issue #4789, but you closed it with blocked status without any discussion.

Oh oops, sorry! I did a dumb and thought it was a PR. That is entirely my fault; will re-open.

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

Successfully merging this pull request may close these issues.

Task: document the deprecation of no-use-before-declare
8 participants