Skip to content
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

Closed
sandersn opened this issue Dec 1, 2016 · 47 comments
Closed
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@sandersn
Copy link
Member

sandersn commented Dec 1, 2016

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

type Flags = {
  aLongName: boolean;
  anotherReallyLongOne: number;
  someOtherOnes: boolean;
  [other: string]: number | boolean;
}

Expected behavior:

function f(flags: Flags) {
  flags.aLongName; // ok
  flags.anUnknownName; // ok
  flags.aLongname; // ok, but misspelled
  flags['aLongName']; // ok
  flags['anUnknownName']; // ok
  flags['aLongname']; // ok, but misspelled
}

Actual behavior:

function f(flags: Flags) {
  flags.aLongName; // ok
  flags.anUnknownName; // error, unknown
  flags.aLongname; // error, misspelled
  flags['aLongName']; // ok
  flags['anUnknownName']; // ok
  flags['aLongname']; // ok, but misspelled
}
@sandersn sandersn changed the title Allow property access syntax for types with string index signatures Dec 1, 2016
@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Dec 1, 2016
@jeffreymorlan
Copy link
Contributor

jeffreymorlan commented Dec 1, 2016

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']).

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]);
}
@gcnew
Copy link
Contributor

gcnew commented Dec 5, 2016

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.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 5, 2016

@gcnew, the proposal is only limited to types that have index signatures, and not all types. does this change your feelings about it?

@gcnew
Copy link
Contributor

gcnew commented Dec 5, 2016

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

@bmhaskar
Copy link

Good suggestion. Will help in declaring types/interfaces for JSON structure which are received via API's

@ratneshsinghparihar
Copy link

Agreed, this proposal make sense.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Dec 14, 2016

TLDR: this is an excellent change. The important thing here is that one should not have to resort to using the any type to access a property.

@jeffreymorlan

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:

I vehemently disagree. They are often heterogeneous and that is what makes them useful.
A common example is a dynamic collection of form fields. Each value has an often dramatically different shape from the others, with completely unrelated properties such as options lists, regex validators, function validators, child fields and so on. Using index syntax to access this value has two purposes:

  1. To access fields dynamically via external key, for example mapping from a property present in the current shape of the form model into the object to retrieve child fields, options, or validators.
  2. To retrieve the value of a statically known key which is a not valid identifier.

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.

@gcnew

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.

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.

Map<string, object> is only homogeneous in the most vacuous sense, it is in fact a static type offering no valuable type safety and a generic instantiation that has no place in a statically typed language that has union types.

it incentivises dot access for hashes, which adds burden on the reader.

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.
As Douglas Crockford said, JavaScript objects are like little hashtables and this is a great thing.

I strongly support this change.

@gcnew
Copy link
Contributor

gcnew commented Dec 14, 2016

@aluanhaddad

Are you saying that they should be using classes for standard types and ES2015 Maps for everything else?

No! All I'm saying is that currently MapLikes are obvious and using dotted properties you are 100% safe. This suggestion blurs the line and chips away type safety. I'm a supporter of adding more guarantees, even if it means giving up some otherwise valid syntax.

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.

@gcnew
Copy link
Contributor

gcnew commented Dec 14, 2016

PS: In a perfect world indexing signatures should always return T | undefined, and definitions like

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`..
@aluanhaddad
Copy link
Contributor

@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 any which is much worse. Obviously it is just syntactic sugar, and not much at that, but it is very true to the nature of JavaScript.

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.

@unional
Copy link
Contributor

unional commented Dec 15, 2016

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.

@abenhamdine
Copy link

abenhamdine commented Dec 25, 2016

As @unional said, I can't imagine this behaviour without an option, since it is a breaking change...

@DanielRosenwasser
Copy link
Member

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

@abenhamdine
Copy link

abenhamdine commented Jan 4, 2017

Ah yes, you're right, it's definitely not a breaking change, sorry.
But the change breaks the expectations of some developpers, so in this way, an option would be a good thing to allow them to stay with the current behaviour.

Anyway, I think this change would be indeed very useful.

@DanielRosenwasser
Copy link
Member

We discussed this in our design meeting today, and most of the team seemed in favor of moving forward.

@DanielRosenwasser DanielRosenwasser added Committed The team has roadmapped this issue and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jan 13, 2017
@IanYates
Copy link

IanYates commented Feb 7, 2017

I agree with @bondz and @gcnew - I tend to use x["propName"] when I know the property may not exist. I may even be using x[varStoringPropName] since I'm using a variable to tell me which property to access. I know it's the same as using x.propName but the former feels more like a "here be 🐉 s" in my codebase and the latter is used 99% of the time since I know there's such a property.

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 nameof implemented 😺

@use-strict
Copy link

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.

@abenhamdine
Copy link

I agree with @bondz and @gcnew - I tend to use x["propName"] when I know the property may not exist. I may even be using x[varStoringPropName] since I'm using a variable to tell me which property to access. I know it's the same as using x.propName but the former feels more like a "here be 🐉 s" in my codebase and the latter is used 99% of the time since I know there's such a property.

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

@wizarrc
Copy link

wizarrc commented Feb 7, 2017

@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 boolean | "StringIndexOnly" | "ObjectOnly".

Option 2: A union type of boolean | Array<FlagOption> where FlagOption = "StringIndex" | "Object". At some point, more options can be added later, and true/false can be deprecated.

Definitions:

  • true allow neither implicit any nor dotted string index access
  • false allow both implicit any and dotted string index access (default)
  • StringIndexOnly/StringIndex allows implicit any but not dotted string index access
  • ObjectOnly/Object allows dotted string index but not implicit any

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.

@massimocode
Copy link

massimocode commented Apr 6, 2017

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

@wizarrc
Copy link

wizarrc commented Apr 6, 2017

@massimocode Maybe they could instead add a per index signature explicit any only override like explicit [index: string]: any; to let the author decide typing enforcement. TypeScript 2.3???

@massimocode
Copy link

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.

@wizarrc
Copy link

wizarrc commented Apr 6, 2017

@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 implicit [index: string]: any; would work better.

@wizarrc
Copy link

wizarrc commented Apr 6, 2017

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.

@massimocode
Copy link

massimocode commented Apr 6, 2017

@wizarrc Explicit properties still work fine. For example:

$rootScope.$on(false); // this gives an error
$rootScope.$one(false); // this does not give an error as it falls back to the indexer of type any

I personally think that the type definition is incorrect, but the following still applies:

  • Updating the type definition means breaking usage for everyone who's relying on the indexer.
    Personally I think this is a good thing but I can also imagine all the TypeScript haters who would have yet another anecdote to tell when they say "typescript is rubish becoz our bild keeps braking" (deliberately spelt like that because that's usually about how far their intellect goes)
  • There are some use cases where a dictionary might have helper methods on it. I've had a look on DefinitelyTyped and there seem to be quite a few valid use cases:
    https://github.com/DefinitelyTyped/DefinitelyTyped/search?utf8=%E2%9C%93&q=%22%5Bindex%3A+string%5D%3A+any%3B%22&type=
    I definitely agree that this is a terrible coding/design practice given javascript's dynamic nature and probably why the ES6 Map has an explicit get(...) method, but having said that there's nothing in JavaScript that prevents this and from what I understand the point of TypeScript is to be able to apply strong typing to as many of the JavaScript usages as possible.
@arogg
Copy link

arogg commented May 4, 2017

Just stumbled accross this because I wondered why the jquery typings would allow something like $('#myid').fadeOutttt('slow') which is not an error.

What if i have a interface like this

interface InterfaceSharedAmonyDifferentProjects
{
  method1():void;
  method2():void;
  [index: string]: any;
}

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 [index: string]: any; signature is added to an interface. The way of using explicit string property access was intuitively cleaner because it had a feel to it that you were doing things manually, so type safety was not expected. If dotted access should now be permitted, it should be done, but I think this is the wrong way. So in this sense it is a "breaking" change, although it does not break any existing code.

@RyanCavanaugh
Copy link
Member

Why does jQuery have that index signature in the first place?

@saschanaz
Copy link
Contributor

saschanaz commented May 8, 2017

The return type of the indexer was originally HTMLElement but changed to any by this PR: DefinitelyTyped/DefinitelyTyped#413

I think someone thought $("body")["0"] was a valid use case years ago.

@RyanCavanaugh
Copy link
Member

We should nuke that from orbit. string -> any indexers are basically a free for all; this is probably hiding a lot of other bugs

@arogg
Copy link

arogg commented May 9, 2017

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 string => any indexers in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/angular/index.d.ts

@evil-shrike
Copy link

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.
But now we lost typing for component Options at all.

I guess that it's unlikely that the feature will be rolled back but please provide an option to revert pre-2.2 behavior.

@wizarrc
Copy link

wizarrc commented May 22, 2017

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

@Ebuall
Copy link

Ebuall commented Jan 29, 2018

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'.
@sandersn
Copy link
Member Author

@Ebuall that's a bug, I think. I opened #21464 to track it.

@Johnz86
Copy link

Johnz86 commented Feb 14, 2018

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.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript