-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Object spread and enumerable properties #13148
Comments
This is unrelated to generics. It has to do with the difference between properties and methods. {
f() {}
} is transpiled into the value {
f: function() {}
} which correctly, as per the ECMAScript specification, has an enumerable own property However, consider this expression which is created via a new class {
f() {}
}() this doesn't have an enumerable own property |
I'm hitting this when trying to use object spread to initialize default values. This works: interface RunOptions {
log: (str: string) => void;
}
const DEFAULT_OPTIONS: RunOptions = {
log: function(str: string): void {
console.log(str);
}
};
function run(command: string, opts: Partial<RunOptions> = {}) {
let options: RunOptions = { ...DEFAULT_OPTIONS, ...opts }
} This doesn't, even though it's semantically identical (the only difference is changing function properties to use concise method syntax): interface RunOptions {
log(str: string): void;
}
const DEFAULT_OPTIONS: RunOptions = {
log(str: string): void {
debug(str);
logOnFailure(str);
}
}
function run(command: string, opts: Partial<RunOptions> = {}) {
let options: RunOptions = { ...DEFAULT_OPTIONS, ...opts }
} It seems important for POJOs to be treated as a "data bag," and concise methods in this context should be treated as an enumerable property that happens to contain a function. |
fwiw, we intentionally made As @tomdale says, object literals are intended to be data bags, with concise methods serving as shorthands, while classes have a clear data/behavior split, which is reflected through enumerability. |
@wycats thank you! I remember having read something to this effect at one point and I tried to find it and link it to this issue but I couldn't find it again. It's great to have the intent and reasoning behind the runtime behavior clarified. |
I had a lot of discussions about this when working on the original proposal at #10727 that got condensed to a couple of comments there. I'll try to pull them together in one place here. First, a summary based on @Conaclos' example:
@Conaclos' example just exposes the boundary where the compiler decides that "this method is not owned". We could extend the fakery and treat methods declared in any object literal as own-enumerable. It would take a little more work but not much. However, @tomdale's example exposes the place where fakery will never get us anywhere. In fact, as @tomdale's example shows, as soon as you decouple types and values, the compiler doesn't have a good way to guess which properties of a type are owned: interface I {
a(): void; // own-enumerable?
b(): void; // or not?
}
class C implements I {
a = () => { }; // own-enumerable
b() { } // non-own-enumerable
} Currently we use syntax to guess: interface I {
a: () => void; // probably own-enumerable
b(): void; // probably not
} This is not great. The best thing you can say about it is that it reflects the syntax you will probably write to create an instance of the type. By the way, the reason that this is important is that in 2.2 you will be able to write generic spread types like Unfortunately, the only way I can think of to really fix this is to make own-ness part of the language. For example, we could change interfaces to only have own properties, and require everything else to come from some special interface A {
// only A's own properties here!
a: number;
}
interface B extends__proto A {
// only B's own properties here!
b: (x: string) => void;
} Or we could add own annotations: interface B {
proto a;
b;
} Although those should really be stackable so the compiler could track how many levels up the prototype chain it should expect to see the property. For example: I don't really think this is a good solution. I actually think fakery and subtle syntax is preferable. If we do start tracking all methods back to their declaration we could have a complex set of heuristics that differ for object literals, classes and interfaces, something like: const o = {
a: () => { } // own-enumerable
b() { } // own-enumerable
}
class C {
a: () => { } // own-enumerable
b() { } // not
}
interface I (
a: () => { } // own-enumerable
b() { } // own-enumerable (Note [1])
} But any rule you make here is (1) guaranteed to be wrong sometimes (2) trivially easy to circumvent. And you have to be able to explain the rules in a reasonable amount of time. [1] Probably people who write an interface that they expect to be used in a spread will also expect only object literals to be spread, not instances of classes. |
@tomdale I don't think I called it out explicitly enough, but in 2.1, you can control own-enumerable in interfaces with a syntactic distinction: interface I {
a: () => { } // own-enumerable
b() { } // not
} I intended it to work that way so in that sense, it is By Design. But it might be better to assume that people who are writing interfaces for use in a spread context want users to spread only object literals: interface I {
a: () => { } // own-enumerable
b() { } // own-enumerable
}
const o: I = { a() { }; b() { } };
const s = { ...o }; // still has `a` and `b`.
const c = new class { a() { }; b() { } };
const s2 = { ...c }; // incorrectly has `a` and `b`, but why did you spread a class instance? And it's more honest because the compiler can't possibly track own-enumerable through an interface, so allowing control of its approximation through a "secret" feature is kind of shady. |
As per the discussion in #13331; properties should be filtered iff they originate from method declarations on a class. |
@sandersn Thank you for the detailed explanation. I was only considering the direct use of an inferred object literal type in a spread expression but you point out how quickly that assumption breaks down - as soon as the types and values are are dissociated. @mhegazy Even if the method originates in a class declaration, the class might be extended by an interface and then implemented through an object literal... declare class A {
m(): void;
}
interface B extends A {
n(): void;
}
const x: B = { m() { }, n() { } };
const y = { ...x }; It comes down to how the value is created, which could be at odds with how it is annotated and seems impossible to track in a meaningful way. Also, given that it is not unheard of for .d.ts files to be updated interface A {
m(): string;
}
declare const A: AStatic;
declare interface AStatic {
new (x: string): A;
} to declare class A {
constructor(x: string);
m(): string;
} this could get a lot trickier because what was once a straightforward refactoring becomes a major change. By the way, it is great to hear that spread types are going to be in the next version! |
Hi :)
EDIT:
As noted by @aluanhaddad, the bug is related to the combination of the shorthand syntax in a literal object and the object spread operator on this object.
TypeScript Version: 2.1.4
Code
First code (compilation success):
Replacement for last 3 lines (compilation failing):
Expected behavior:
Replace
Object.assign
with the object spread operator should not be harmful.Actual behavior:
The first code makes happy the compiler.
However if I I replace
Object.assign
with the object spread operator (See replacement code), the compiler raises the next error:The text was updated successfully, but these errors were encountered: