Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Used undefined instead of null. #565

Merged
merged 3 commits into from
Oct 16, 2018
Merged

Used undefined instead of null. #565

merged 3 commits into from
Oct 16, 2018

Conversation

reduckted
Copy link
Contributor

Fixes #490.

I've changed the code to use undefined instead of null, except where it's actually needed, which was only a handful of places:

  • Creating an object without a prototype here
  • Checking the results of regular expression tests here, here and here

There's one thing I'm not sure if I should change (I've left it unchanged for now). The additional_rule_metadata.json and recommended_ruleset.js files both suggest turning the no-null-keyword off. Given that it's now turned on, do those files need to change?

Copy link

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

This looks fantastic. I'm really excited to get it in!

Can you just replace options: undefined back to options: null?

@@ -17,7 +17,7 @@ export class Rule extends Lint.Rules.AbstractRule {
ruleName: 'chai-prefer-contains-to-index-of',
type: 'maintainability',
description: 'Avoid Chai assertions that invoke indexOf and compare for a -1 result.',
options: null,
options: undefined,

Choose a reason for hiding this comment

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

This one always confused me. TSLint itself describes the rule's options as:

Schema of the options the rule accepts.
The first boolean for whether the rule is enabled or not is already implied.
This field describes the options after that boolean.
If null, this rule has no options and is not configurable.

https://github.com/palantir/tslint/blob/237fd905a9fb3c9ca9325d2da6e8145e4a44fe51/src/language/rule/rule.ts#L64

...so we should probably default this to null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, you're absolutely right. I had a look at that comment as well as how the options are used in TSLint before I made the changes and it looked like it shouldn't matter if undefined is used, but of course the one place I didn't look at was what TSLint uses for their own rules, and they use null.

@JoshuaKGoldberg
Copy link

The additional_rule_metadata.json and recommended_ruleset.js files both suggest turning the no-null-keyword off

Yes, let's turn them back on there. Those files are a little older.

Copy link

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

👍

@JoshuaKGoldberg JoshuaKGoldberg merged commit 4e70ba5 into microsoft:master Oct 16, 2018
@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0-beta0 milestone Nov 6, 2018
@astorije
Copy link
Contributor

astorije commented Dec 14, 2018

@JoshuaKGoldberg, I'm having a lot of issues with this change in the recommended ruleset.

As much as I would love to stick to undefined in favor of null in our own codebase, the reality is that a lot of (popular) libraries out there do not support this.
You noted that TSLint itself has a different behavior on null/undefined, it's also true for react-apollo (and other Apollo-related libraries like apollo-cache-inmemory), and I'm sure I'll discover plenty more as I'm updating this plugin in our repos.
I'm also using https://github.com/godmodelabs/flora-sql-parser which strives on null values. While it's a small library and I could disable this rule on a per-file/repo basis, it is built upon https://github.com/pegjs/pegjs which is much more widely used, and generates a ton of nulls everywhere.

Are you at all opposed to removing this from the recommended ruleset?

@reduckted
Copy link
Contributor Author

@astorije I could disable this rule on a per-file basis...

You can turn the rule off in the config file and still use the recommended ruleset.

{
    "extends": ["tslint-microsoft-contrib"],
    "rulesDirectory": ["node_modules/tslint-microsoft-contrib"],
    "rules": {
        "no-null-keyword": false
    }
}

As the readme says,

To start, you can enable our recommended defaults...You can then disable rules you don't find useful.

@astorije
Copy link
Contributor

@reduckted, thanks, we're doing that for a few rules indeed.

FWIW, I do find this rule useful, I just don't think it should be recommended/enabled by default. Many packages out there do not play nice with this, and as we noticed on our end, fixing violations recommended by this rule on things outside of our control (dependencies) will often break.

@reduckted reduckted deleted the 490-stop-using-null branch December 30, 2018 11:10
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.

3 participants