-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow property (dotted) access syntax for types with string index signatures #12596
Comments
It makes a lot of sense to me - dotted access is used when the programmer expects the property to have some particular meaning. A map (string-indexed type) is homogeneous, thus the expectation is wrong: var stuff: { [key: string]: number };
for (var i = 0; i < stuff.length; i++) { // oops, this is nonsense - the "length" property
// doesn't have any special meaning on this object
console.log(stuff[i]);
} |
I'm strongly against this change. I don't have data for modern JS usage but it used to be that when people used objects as hash tables they preferred indexing access. It might have been because of experience with more rigid languages but I believe the real reason was because it is a different logical use and indexing conveys it to the reader. Also, if this gets merged in we'll lose valuable type checking. My plea is if you decide to go for it, please add one of your "favourite" flags to turn it off. Edit: My point is that currently we trade some otherwise valid syntax for additional type safety. And that's good - yes, it is a convention but it is in line with the type signature. On the type level properties are declared naked and the value level counterpart is also naked dot access. Index signatures are declared inside brackets - you access them with brackets. |
@gcnew, the proposal is only limited to types that have index signatures, and not all types. does this change your feelings about it? |
@mhegazy No. The reason is twofold: objective - we'll lose some static typing (spelling mistakes / refactoring leftovers of "known" properties will no longer be hard checked) and subjective - it incentivises dot access for hashes, which adds burden on the reader. |
Good suggestion. Will help in declaring types/interfaces for JSON structure which are received via API's |
Agreed, this proposal make sense. |
TLDR: this is an excellent change. The important thing here is that one should not have to resort to using the
I vehemently disagree. They are often heterogeneous and that is what makes them useful.
When writing JavaScript, I use the indexer syntax only when a key is not valid identifier. This was actually probably the most difficult part of adopting TypeScript for me, I got over at like 4 years ago but it is still annoying. Fundamentally it is far less readable to have to use the string syntax.
Even if this is true, I don't see much value in it. It reflects, as you point out, experience with statically typed languages like Java that are unable to express flexible patterns like heterogeneous collections in a typesafe fashion.
I'm not sure what you mean by this. Are you saying that they should be using classes for standard types and ES2015 Maps for everything else? ECMAScript classes are not good enough. They will get better over time, but right now they are very very weak abstraction mechanism. On the other hand maps offer no first class syntax. I strongly support this change. |
No! All I'm saying is that currently The following stays the same: type Coloured<T> = {
colour: 'red' | 'green' | 'blue',
value: T
}
declare const c: Coloured<string>;
c.color // Error: typo mistake However this will change in a negative way: type WithColour = {
colour: string,
[key: string]: string | undefined
}
declare const d: WithColour;
d.color // BAD! Currently an error, will no longer be
d['prop'] // it's obvious we venture into the unknown
d.prop // BAD: Does such a property exist? We'll no longer be sure :( For me there are two distinct uses of JavaScript objects. One is for structured data - the properties are well know and enumerated in advance. The other is for Maps/Dictionaries/Hashes/you name it. This second one is completely different - we do dynamic lookup based on arbitrary keys for which a value might or might not exist. To be allowed to do this we provide an indexing signature on the type. For me it's only natural to use this very same indexing for access as well. It also conveys the idea that the object is a Map and there is a good chance a corresponding value is not being present. |
PS: In a perfect world indexing signatures should always return type WithColour = {
colour: string,
[key: string]: string | undefined
} should be illegal. The above definition can easily be broken: declare const d: WithColour;
const key: string = true ? 'colour' : '';
d[key] = undefined;
d.colour // says type `string` but the value is actually `undefined`.. |
@gcnew I understand why you feel that way, but it is cumbersome to be required to use subscripting syntax to access a "dynamic feeling js object". The only alternative is resorting to That said, however, I appreciate the argument that the reduced safety might not be worth it, especially as type inference becomes more and more powerful, the need for dynamic dotted access becomes less and less of an issue. |
I can see the argument on both, and feel both. Can we have a option to toggle that? {
"compilerOptions": {
"allowDottedSyntaxForIndexes": true/false
}
} EDIT: @gcnew actually already mentioned this in his first comment. |
As @unional said, I can't imagine this behaviour without an option, since it is a breaking change... |
@abenhamdine it's not a breaking change - it's actually more permissive. More code will work, however, it's not necessarily clear to me at this time whether everyone's on board with the idea. |
Ah yes, you're right, it's definitely not a breaking change, sorry. Anyway, I think this change would be indeed very useful. |
We discussed this in our design meeting today, and most of the team seemed in favor of moving forward. |
I agree with @bondz and @gcnew - I tend to use I know it's only for certain types, and admittedly I don't use those types that often, but it does feel like something best put behind a flag if possible. I'd rather see |
As @abenhamdine said, this change breaks the expectation of developers, but I would not add Yet Another Flag (TM) though. The flag situation is bad enough as it is To be honest, even if I don't support this change, having an object that has both named properties and an index signature is a mix of concerns in the way that @gcnew explained. You either have a dictionary-like data structure, where the keys are data, or you have a structured object, where each property is known in the code and specially handled. Having a hybrid is just a source of WTFs/min, regardless of how TypeScript decides to handle it. |
Exactly, that's why it would have been great to distinguish the two differents cases on a per-object basis, and not in the whole code (without consideration of a global flag). |
@use-strict I think the point of this addition is to be more idiomatic JavaScript to the native developer. This seems like a point where the dynamic nature of JavaScript is clashing with the type correctness that Typescript brings us. @sandersn Instead of adding yet another flag, what do you think about changing the type of the noImplicitAny flag? Option 1: A union type of Option 2: A union type of Definitions:
This way it would reduce the number of flags needed in the tsconfig.json file and allow developers to gradually pick their level of enforcement. |
Please see this as well: DefinitelyTyped/DefinitelyTyped#15683 @DanielRosenwasser I feel like if the name of a property is known at compile time, then it can be hard coded into the type def/interface, and if it is not known at compile time, then it can be accessed via the indexer. Exactly what was said here: #12596 (comment). We're definitely losing type safety here. I really really really want to see a compiler option to disable this "extra permissiveness". |
@massimocode Maybe they could instead add a per index signature explicit any only override like |
That's definitely one possible approach but it would also mean updating loads of different type definitions and also getting into arguments with people about whether or not their typings should be explicit or not. Also technically probably more difficult to implement. For me, a simple compiler flag would be awesome. That way different teams/projects can use different settings to suit their coding styles/preferences. |
@massimocode They seem to be pretty much against having explicit any with index signature because it balances some internal sense of equality. I think your link shows an excellent example of where it fails. In that example, do explicit properties like $broadcast lose type definition? Maybe instead |
They allow operator overloads in languages like C# for implicit object assignment without boxing/unboxing issues. Syntax like that would make sense for any type of implicit conversion. In this case, it's implicit conversion to any. |
@wizarrc Explicit properties still work fine. For example:
I personally think that the type definition is incorrect, but the following still applies:
|
Just stumbled accross this because I wondered why the jquery typings would allow something like What if i have a interface like this
How is type safety enforcement supposed to work if different developers work on this interface in different projects? E.g. dev 1 changes or removes some method signatures and dev 2 does not get compiler errors when he uses the new version of the interface in his project. That can be a nightmare to fix if lots of things have changed. Or what about type definitions (e.g. jquery uses indexers)? When method signatures are changed, the user does not get type errors. And what about people who write method calls without using autocompletion? Typo = error. As said, type safetly of all jquery objects are now compromised. People are now forced to use autocompletion for jquery objects in order to avoid typos and cannot be as sure as before that their jquery code still works if jquery and its typings are updated. I am sure jquery is not the only library suffering here. You cannot simply force the jquery devs and all other library devs to not use indexers, this is simply not an option! Please at least acknowledge a problem here. Also, I think it is not conceptually "pure" for type safety to break completely once a |
Why does jQuery have that index signature in the first place? |
The return type of the indexer was originally HTMLElement but changed to I think someone thought |
We should nuke that from orbit. |
Maybe the compiler should give a warning if an interface exposes members that are assignable to an indexer from the same interface in order to avoid such fatal problems in the future. Of course better solutions may exist :) These kind of problems are easy to produce and hard to find (sort of like empty catch blocks), so I think just removing the any indexer from jquery is a neccessary fix, but not sufficient to solve the whole problem. edit: this problem is rampant. just saw multiple |
I believe this change made more evil then good. What I can't understand why it wasn't introduced via an compiler option, or at least an options for old behavior was provided. It's too nuclear change. You may say "just remove index signature". But in our case it's not easy to do. We have a hierarchy of UI components with corresponding Options interfaces for them (a component's constructor accept an object of component's Options interface). But there're also mixins which provide shared logic for components. And sometimes there should be set options for a component with options for mixin and they are not part of component Options interface. So base component class has indexing signature to allow extensibility. I guess that it's unlikely that the feature will be rolled back but please provide an option to revert pre-2.2 behavior. |
@evil-shrike Can you provide code examples of how your components have been affected by this change? Consider opening a separate issue referencing this thread so that it can be properly triaged. |
Not sure if this is a bug or intended. I have a type with index signature: interface SafeNull {
(...args: any[]): SafeNull
[key: string]: SafeNull
}
safeNull.toUpperCase() // Ok
"".toUpperCase() // Ok
let y: string | SafeNull = Math.random() < 0.5 ? "" : safeNull
y.toUpperCase() // Error
// Property 'toUpperCase' does not exist on type 'string | SafeNull'.
// Property 'toUpperCase' does not exist on type 'SafeNull'. |
What I am missing is something like: type dotKeys = "key.1" | "key.2" | "complex.key.defined.here" | "simple.key";
interface HasKeysWithDot{
[x: dotKeys]: string;
} or even better type PropertyKeys = "normal" | "key.test" | "design.long.test" | "with.number.1";
interface DefinedKeys<T> {
[x:T]: string;
}
let objectWithDotPropertyKeys: DefinedKeys<PropertyKeys> = {
"normal": "test",
"key.test": "test2",
"design.long.test": "test3"
} Assigning object with dot keys is possible approach in javascript. There should be some typings for this. |
It doesn't make much sense to forbid property access (
o.unknown
) syntax on a type with a string index signature, but allow element access syntax (o['unknown']
).Note that this should only apply to types with an explicit string index signature — element access is currently allowed on any type if you don't pass
--noImplicitAny
, but this should not be true for property access.Given
Expected behavior:
Actual behavior:
The text was updated successfully, but these errors were encountered: