feat(eslint-plugin): [dot-notation] optionally allow square bracket notation where an index signature exists in conjunction with noPropertyAccessFromIndexSignature
#3361
Conversation
…noPropertyAccessFromIndexSignature
|
Thanks for the PR, @djcsdy! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
There was a problem hiding this comment.
almost there! the logic is looking pretty good so far.
One thing I think we should block is accessing a known property via the computed notation. Known properties should be accessed via dot notation.
interface SomeType {
foo: string;
[propName: string]: any;
}
function doStuff(value: SomeType) {
let x = value["someProperty"]; // should NOT error
let y = value.foo; // should NOT error
let z = value['foo']; // should error!!!
}I think the rule should be strict about this because the rule is intended to ensure you're using dot notation whenever you can.
| objectType, | ||
| ts.IndexKind.String, | ||
| ); | ||
| if (indexType != undefined) { |
There was a problem hiding this comment.
nitpick prefer comparisons against null
| if (indexType != undefined) { | |
| if (indexType != null) { |
There was a problem hiding this comment.
One thing I think we should block is accessing a known property via the computed notation. Known properties should be accessed via dot notation.
This is already blocked, see the tests.
|
Sorry, to clarify. There are two cases here. The first is the case of a known property on a type with no index signature. interface Foo { prop: string }
declare const x: Foo;
x.prop // ✅ rule does not reportThe second case is the case of a known property on a type with an index signature. interface SomeType {
foo: string;
[propName: string]: any;
}
function doStuff(value: SomeType) {
// should NOT error if the compiler option is on
// your code correctly handles this ✅
let x = value["someProperty"];
// this case should not error ever!
// your code correctly handles this ✅
let y = value.foo;
// this case SHOULD error!
// your code does not currently error on this
let z = value['foo'];
}This is a bit of a trickier case to handle as you'll have to look up the property in the base type to see if it exists. |
Codecov Report
@@ Coverage Diff @@
## master #3361 +/- ##
==========================================
+ Coverage 92.85% 92.87% +0.01%
==========================================
Files 318 318
Lines 11048 11056 +8
Branches 3128 3132 +4
==========================================
+ Hits 10259 10268 +9
Misses 352 352
+ Partials 437 436 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I understood what you meant. I believe it is already handled, see this test case: https://github.com/typescript-eslint/typescript-eslint/pull/3361/files#diff-93cb24cbe290e1f30e97aaf0631ade9bd3751820d1f0bef8ecb4a0532243e2bdR302-R323 However I will double check, perhaps the different type signatures between the index type and the property are confusing matters in this test case. |
|
I'm so sorry - I wasn't reading your code or tests correctly. |
noPropertyAccessFromIndexSignature
|
@djcsdy @bradzacher Hey! I think there is a bug when there is an optional chaining operator: foo?.bar['buzz'] // unexpected eslint errorCan you please have a look? |
|
Please file an issue. PRs are not the place for discussing bugs. |
Fixes #3104.
This PR modifies the
dot-notationrule to add a new option namedallowIndexSignaturePropertyAccess.When this option is set to
true,dot-notationallows the use of square bracket notation if there is a corresponding index signature defined on the type that is being dereferenced. For example:If the TypeScript compiler option
noPropertyAccessFromIndexSignatureis set totrue, then thedot-notationrule always behaves as ifallowIndexSignaturePropertyAccessis set totrue. The motivation for this behaviour is thatnoPropertyAccessFromIndexSignaturecauses the TypeScript compiler to require the use of square bracket notation, so typescript-eslint conflicts with the compiler if it disallows square bracket notation in this case. See #3104.