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

Speculation about #12596 Allow property (dotted) access syntax for types with string index signatures #16027

Closed
evil-shrike opened this issue May 23, 2017 · 14 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@evil-shrike
Copy link

Continue discussion on #12596 Allow property (dotted) access syntax for types with string index signatures (introduced in TS 2.2).

Let's consider a component with options.

class MyComponent {
    static defaultOptions: MyComponent.Options = {
        presentation: "dropdown"
    };
    options: MyComponent.Options;
    constructor(options: MyComponent.Options) {
        this.options = utils.append(this.options, options, {deep: true});
    }
}
namespace MyComponent {
    export interface Options {
        presentation: "dropdown" | "inline";
    }
}

All options the component uses are declared in its interface.
So to create an instance:

let c = new MyComponenent({ presentation: "inline" });

Now, imagine that component accepts a callback:

    export interface Options {
        presentation: "dropdown" | "inline";
        onBeforeRender: () => void;
    }
let c = new MyComponenent({ 
    onBeforeRender: () => {
    }
});

Let's imagine that inside onBeforeRender we need access to some value passed from parent content.

let c = new MyComponenent(utils.append({ 
    onBeforeRender: () => {
        let val = this.options["theValue"];
    }
}, {
    theValue: theValueFromParent
});

So we're adding an aspect to class and it depends on some data provided by a different aspect.
The easiest way to implement something like this was to add index signature into Options:

namespace MyComponent {
    export interface Options {
        [key: string] => any;
    }
}

It allowed us to extend components without subclassing.
Now (since TS 2.2) it has very serious impact - we lost type checking for Options type at all. The worst thing is that it could be not very clear for people migrating from previous versions. Everything still works. No breaking changes. But accidentally one can notice then compiler doesn't help anymore:

class MyComponent {
    static defaultOptions: MyComponent.Options = {
        prisintetion: "dropdown" // should be error but it's not
    };
}

So, probably we have two different cases here: a type as map and a type with optional extensibility.

I'd suggest to introduce optional index signature which implements the previous behavior:

    export interface Options {
        [key: string]? => any;
    }

this would mean the same as before:

let opt: Options;
opt.noExisting;     // error
opt["noExisting"]; // ok
@mhegazy
Copy link
Contributor

mhegazy commented May 23, 2017

Other than this is how it used to work in TS, Why should these two be different?

opt.noExisting;     
opt["noExisting"]; 

@evil-shrike evil-shrike changed the title Speculation obout #12596 Allow property (dotted) access syntax for types with string index signatures Speculation about #12596 Allow property (dotted) access syntax for types with string index signatures May 23, 2017
@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels May 24, 2017
@RyanCavanaugh
Copy link
Member

This also has impacts for certain minifiers. I think there's a strong argument to allow opting in to the classic behavior (certainly I'd run with that flag on!).

@evil-shrike
Copy link
Author

Why should these two be different?

They are different in any case. opt["noExisting"] work both in 2.1 and 2.2 regardless of existing of index signature.
So adding index signature affected only passing a value not using it.

This also has impacts for certain minifiers.

Sorry, don't get it. What does have impact?

@evil-shrike
Copy link
Author

interface Options1 {
	name?: string;
}

interface Options2 {
	name?: string;
	[key:string]: any;
}

function f1(opt: Options1) {
	// Work both in TS 2.1 and 2.2
	opt["any"];
}

function f2(opt: Options2) {
	// Works both in TS 2.1 and 2.2
	opt["any"];

	// Works only in >=2.2
	//opt.any;
}

function main() {
	// doesn't work both in TS 2.1 and 2.2: 
	// 	error TS2345: Argument of type '{ some: string; }' is not assignable to parameter of type 'Options1'.
	// 	Object literal may only specify known properties, and 'some' does not exist in type 'Options1'.
	//f1({some: "some"});

	// Work both in TS 2.1 and 2.2
	f2({some: "some"});
}

@mhegazy
Copy link
Contributor

mhegazy commented May 24, 2017

Your answer is it was working this way in TS 2.1, but my question was "Other than this is how it used to work in TS, Why should these two be different?"

@evil-shrike
Copy link
Author

I'm sorry, but I understood you as that you think they should be the same. But even we forget about the fact how it was in TS 2.1 these two expressions are in fact different (in 2.3) for any type without index signature. So I can ask the same question: why?
And if they are different for types w/o index-signature why should they be same for types w/index-signature?

@dmitrysteblyuk
Copy link

dmitrysteblyuk commented Jun 23, 2017

Is there an option to disable dotted syntax as it was before? I'm using Closure Compiler for angular 2 app and now I have to manually go through all the index-signature types to check if I access properties with quotes. Otherwise closure will rename those properties and I'll have errors at runtime.

In angular it is ActivatedRoute.params, ActivatedRoute.queryParams, FormGroup.controls, FormGroup.value, ValidationErrors etc whose properties must be accessed with quoted syntax only. Also all the json data received from external resources which is typed as index-signature cannot be used with dot syntax.

@troywweber7
Copy link

This was working before... Sort of annoying that its now not working...

@mhegazy
Copy link
Contributor

mhegazy commented Aug 21, 2017

A type { a: number } has a property a. you can access it using x.a or as x["a"], and in both cases the type of the property is number.

Similarly a type with an index signature, { [x:string]: number }, says it can be accessed with any string, and the property type is always a number, so it can be accessed as x.foo or x["foo"].

A type with no explicit member declaration or index signatures has no properties. e.g.{}. in this case, both x.a and x["a"] are considered errors (though the later is only reported under --noImplicitAny). you could argue that both should be the same, but that would be most property accesses.

The change only allows types with index signatures. I view this change as fix to an inconsistency in the system.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 22, 2017

This was working before... Sort of annoying that its now not working...

One person's not working is another person's fix of a limitation.

@dmitrysteblyuk
Copy link

@mhegazy, @kitsonk Typescript is widely used by Angular 2 developers.
The only compiler that can treeshake angular 2 modules is Google Closure (angular 2 bundles are really big and closure makes them twice smaller than other minifiers (in my case at least)).
It is a huge difference for Google Closure between { 'a': ... } and { a: ... }, x['a'] and x.a.
So providing an option to disallow dot access for types with index signatures and quote access for types w/out index signatures could be really beneficial, the way I see it.

@kitsonk
Copy link
Contributor

kitsonk commented Aug 22, 2017

So providing an option to disallow dot access for types with index signatures and quote access for types w/out index signatures could be really beneficial, the way I see it.

That is a linting issue, not a typing/emit issue. If you project needs that rule, then it can be added.

This has already been raised and discussed with the Angular 2 team... See #15206 (in particular this comment). Also, there is still an open proposal by the Angular 2 team at #14267 which is in this realm.

@dmitrysteblyuk
Copy link

kitsonk thanks, you're right, #14267 is the issue I need.

@RyanCavanaugh
Copy link
Member

This is implemented with noPropertyAccessFromIndexSignature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants