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

Objects with string indexing should be compiled with actual string indicies #15206

Closed
mrmeku opened this issue Apr 15, 2017 · 13 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@mrmeku
Copy link

mrmeku commented Apr 15, 2017

TypeScript Version: 2.2.1 / nightly (2.2.0-dev.201xxxxx)

Code

class Foo {
    constructor(public value: { [prop: string]: boolean }) {}
}
const foo = new Foo({ a: true });

Expected behavior:
The compiler should generate an object with string properties (notice how a is in quotes below). This behavior allows for proper minification of the generated code since minifiers such as the closure compiler will not rename strings.

var Foo = (function () {
    function Foo(value) {
        this.value = value;
    }
    return Foo;
}());
var foo = new Foo({ 'a': true });

Actual behavior:
Unfortunately, even thought the type of the object specified that all boject properties are strings, the generated code does not wrap the property name in quotes.

var Foo = (function () {
    function Foo(value) {
        this.value = value;
    }
    return Foo;
}());
var foo = new Foo({ a: true });
@mrmeku mrmeku changed the title Objects with string indexing should be compiled with string indexe Objects with string indexing should be compiled with actual string indicies Apr 15, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Apr 15, 2017

What problem is this causing? Traditionally strings as keys of object literals are only quoted when the string contains something that would not be a valid variable name in JavaScript (e.g. 'a key like this').

@mrmeku
Copy link
Author

mrmeku commented Apr 15, 2017

The problem is that without using actual strings minifiers are prone to rename the symbol. This becomes a problem when receiving json's via http for example where the server constructs a message with the non renamed symbol

@kitsonk
Copy link
Contributor

kitsonk commented Apr 16, 2017

Most minifiers do not mangle object literal keys unless they are instructed it is safe to do so because of exactly the problem you mention.

The developer has every right to specify object literals as strings or not, if that meets their particular use case. Having TypeScript universally enforce one way or the other would likely not be a good idea.

@mrmeku
Copy link
Author

mrmeku commented Apr 16, 2017 via email

@kitsonk
Copy link
Contributor

kitsonk commented Apr 17, 2017

Imagine a team trying to enforce that a specific object only has its properties accessed using bracket notation.

Again, what problem would this be causing? If anything, it would appear to be a stylistic choice then, which is the domain of a linter.

@mrmeku
Copy link
Author

mrmeku commented Apr 17, 2017

I'm using Angular 2 with the closure compiler. Angular's form's has an implicit requirement that FormGroups values be access with string indicies so that the closure compiler does not minify them.

@mprobst
Copy link
Contributor

mprobst commented Apr 17, 2017

By the way, AFAIU this is not limited to Closure compiler, you'll have the same problem when using Uglify in advanced mode.

@tbosch
Copy link

tbosch commented Apr 17, 2017

See also #14267 files by @alexeagle

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 17, 2017
@RyanCavanaugh
Copy link
Member

For any code that is "normal" JS, we're going to emit the same code as what you wrote. The onus is on the developer to have the right rules / process around whether or not the code they're writing is correct for downstream heuristic-based tools.

@mprobst
Copy link
Contributor

mprobst commented Apr 19, 2017 via email

@kitsonk
Copy link
Contributor

kitsonk commented Apr 19, 2017

One person's improvement is another's regression.

It appears that people were depending on a side-effect versus a feature.

@RyanCavanaugh
Copy link
Member

It should be very simple to write a TSLint rule to detect "used dotting to access string index signature" (or vice versa) mismatches, assuming one doesn't exist already.

@mhegazy
Copy link
Contributor

mhegazy commented May 8, 2017

This issue seems to have been already addressed or is unactionable at the moment. I am auto-closing it for now.

@mhegazy mhegazy closed this as completed May 8, 2017
mprobst added a commit to angular/tsickle that referenced this issue May 13, 2017
TypeScript 2.3 allows users to use `dotted.access` for types that (only) have a
string index type declared, e.g. `{[k: string]: ...}`. This breaks renaming in
tools such as Closure Compiler (which tsickle targets), as some locations might
access the property with quotes, some without.

This is an incomplete fix: TypeScript allows assigning values with properties of
the matching type into types with index signatures. Users can then access the
original value with dots, but the aliased value with quotes, which breaks, too.
This is probably not fixable without a global aliasing analysis of the program.

See also:
    microsoft/TypeScript#14267
    microsoft/TypeScript#15206
mprobst added a commit to angular/tsickle that referenced this issue May 17, 2017
TypeScript 2.3 allows users to use `dotted.access` for types that (only) have a
string index type declared, e.g. `{[k: string]: ...}`. This breaks renaming in
tools such as Closure Compiler (which tsickle targets), as some locations might
access the property with quotes, some without.

This is an incomplete fix: TypeScript allows assigning values with properties of
the matching type into types with index signatures. Users can then access the
original value with dots, but the aliased value with quotes, which breaks, too.
This is probably not fixable without a global aliasing analysis of the program.

See also:
    microsoft/TypeScript#14267
    microsoft/TypeScript#15206
mprobst added a commit to angular/tsickle that referenced this issue May 18, 2017
TypeScript 2.3 allows users to use `dotted.access` for types that (only) have a
string index type declared, e.g. `{[k: string]: ...}`. This breaks renaming in
tools such as Closure Compiler (which tsickle targets), as some locations might
access the property with quotes, some without.

This is an incomplete fix: TypeScript allows assigning values with properties of
the matching type into types with index signatures. Users can then access the
original value with dots, but the aliased value with quotes, which breaks, too.
This is probably not fixable without a global aliasing analysis of the program.

See also:
    microsoft/TypeScript#14267
    microsoft/TypeScript#15206
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants