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 setter type to be incompatible with the getter type #43662

Closed
5 tasks done
saschanaz opened this issue Apr 13, 2021 · 22 comments Β· Fixed by #53417
Closed
5 tasks done

Allow setter type to be incompatible with the getter type #43662

saschanaz opened this issue Apr 13, 2021 · 22 comments Β· Fixed by #53417
Assignees
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Suggestion An idea for TypeScript

Comments

@saschanaz
Copy link
Contributor

Suggestion

πŸ” Search Terms

differing accessor types

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

#42425 forces the getter type to be assignable to the setter type. This makes the feature limited on the following DOM typing case, and thus it could be great if the limitation can be lifted.

πŸ“ƒ Motivating Example

[Exposed=Window]
interface CSSStyleRule : CSSRule {
  attribute CSSOMString selectorText;
  [SameObject, PutForwards=cssText] readonly attribute CSSStyleDeclaration style;
};
interface CSSStyleRule extends CSSRule {
    selectorText: string;
    get style(): CSSStyleDeclaration;
    set style(cssText: string); // currently an error
}
document.body.style = "display: none" // thus still an error while the actual behavior allows it πŸ€”

πŸ’» Use Cases

Allows Web IDL readonly attributes to be assignable, and thus the DOM typing better matches the actual behavior.

Transferred from microsoft/TypeScript-DOM-lib-generator#996.

@andrewbranch
Copy link
Member

I don’t know if #2521 was intended to be closed, or if it remains open to track feedback like this. Also related is #43066, but this case is one where you can’t just make the set type string | CSSStyleDeclaration because assigning a CSSStyleDeclaration back to the accessor doesn’t work 😬

@DanielRosenwasser DanielRosenwasser added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Apr 14, 2021
@thw0rted
Copy link

In the "Restrictions" section near the top of #42425, Ryan said that "obj.x = obj.x; must be legal assuming obj.x is writable at all" in order to "prevent novel unsoundness from occurring". Smarter people than I will have to tell you exactly why that's so but it also seems to align with the Goals:

  • Goal: Statically identify constructs that are likely to be errors. -- designing an API that does not allow x=x sure seems "likely to be an error"
  • Non-goal: Introduce behaviour that is likely to surprise users -- the DOM violated the POLS here, and it should feel bad.

Also, in case it matters, el.style = el.style might not do what you want, but it does not cause a runtime error.

@Pentadome
Copy link

Why not just make a Get..() and Set...() function? Getters and setters are supposed to be so simple that they are similar in use as fields.

@saschanaz
Copy link
Contributor Author

saschanaz commented Apr 16, 2021

Also, in case it matters, el.style = el.style might not do what you want, but it does not cause a runtime error.

That should still be a static error because it's virtually el.style = el.style.toString() where the string becomes [object CSS2Properties].

@andrewbranch
Copy link
Member

Also, in case it matters, el.style = el.style might not do what you want, but it does not cause a runtime error.

I mean, it will literally never do what you want or expect, unless you just want to watch the world DOM burn. Preventing runtime errors is kind of priority one, but preventing blatantly obvious behavioral errors is also a goal.

@shellscape
Copy link

Lending my support to this proposal. Thankfully language ability trumps subjective opinion. If we wanted to write a bunch of getThing and setThing methods, we'd probably be in Java.

@rbalicki2
Copy link

rbalicki2 commented Oct 22, 2021

Hi y'all, I'd like +1 this proposal as well. In Relay (relay.dev), we are working on a feature where graphql linked fields can be updated with simple assignment, instead of the ugly existing API recordProxy.setLinkedRecord(...). A simplified example would be:

const updatableData = readUpdatableQuery(graphql`
  query FooQuery @updatable {
    best_friend {
      ...AssignableBestFriend_user
      name
    }
  }
`, store);
// the type of updatableData would be as follows
type UpdatableData = {
  get best_friend(): { name: ?string },
  set best_friend(value: { $fragmentRefs: AssignableBestFriend_user }): void,
}

This allows us to enforce that any user that can be assigned to best_friend must have had the AssignableBestFriend_user fragment spread at that location. Essentially, using this mechanism, we are letting the Relay compiler validate assignments. However, we don't want to enforce that any field that is assigned to best_friend must also have a name selection, or to include the $fragmentRefs field from the getter.

(This is only part of what is required to ensure that the assignment is valid, but the rest is irrelevant for the getter/setter discussion.)

Thank you! It would be a pity to be able to roll this out with full type safety for users of Flow, but to have limited type safety in Typescript.

@valerii15298
Copy link

+1 to this proposal. Using apollo + graphql on front end. And such functionality would be really VERY helpful.
Using Apollo type policies I am creating smal library helper to better usability with help of Javascript proxy api. So basically when i need to get value with some parameters it will be a function(when invoked(with arguments or not, will return value) and set to set value.
I am using this Generic:

type RecursiveFunc<T> = {
  [P in keyof T]: T[P] extends (infer U)[]
    ? RecursiveFunc<U>[]
    : T[P] extends object
    ? (() => RecursiveFunc<T[P]>) | T[P]
    : (() => T[P]) | T[P];
};

const conn: RecursiveFunc<Connection> = someval

// get all ports that connected to node 1
conn.ports({args: {node: 1}})

// set ports
conn.ports = [ {node: 1}, {node: 2} ]

// TS2349: This expression is not callable.
// Not all constituents of type 'Ports | (() => RecursiveFunc<Ports>)' are callable.
// Type 'Port' has no call signatures.

Though I am using Javascript Proxy API and it will work in both cases(with get and set) with corresponding types.

@jlalmes
Copy link

jlalmes commented Jan 20, 2022

+1 for this feature. Flagging this issue that contains lots of great use cases for incompatible getter/setter types: #2521

@josh-
Copy link

josh- commented Oct 4, 2022

Just wanted to flag another Web API that isn't able to be used without this feature:

The URL.search getter returns string, but the setter also accepts a URLSearchParams object, so the following fails to compile:

const url = new URL("https://example.com");
url.search = new URLSearchParams({ test: "test" });

with error TS2322: Type 'URLSearchParams' is not assignable to type 'string'., despite it being valid.

@saschanaz
Copy link
Contributor Author

Just wanted to flag another Web API that isn't able to be used without this feature

Your code is equivalent to this:

const url = new URL("https://example.com");
url.search = new URLSearchParams({ test: "test" }).toString();

... as URLSearchParams has special stringification behavior. So no, it can be used without this feature.

@nopeless
Copy link

nopeless commented Oct 7, 2022

I feel like the pot holes generated by having different get/set types can also be overcome by TypeScript.

I believe most people are going to support same types and over load them to be different as well for example

get id(): string {}
set id(): string | number {}

string | number extends string, so even JavaScript developers who are expecting get and set to be the same type shouldn't have any issues

Edit: This argument is invalid. This is the default behavior.

JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 13, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 13, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 21, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 27, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 27, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
JeanMeche added a commit to JeanMeche/angular that referenced this issue Dec 29, 2022
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
alxhub pushed a commit to angular/angular that referenced this issue Jan 5, 2023
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
alxhub pushed a commit to angular/angular that referenced this issue Jan 5, 2023
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
alxhub pushed a commit to angular/angular that referenced this issue Jan 5, 2023
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
@mrflip
Copy link

mrflip commented Jan 17, 2023

What are the blockers for making this happen? The error when this rule is violated seems like a nice-to-have, but when the narrowing is intentional it is paralyzing.

If the feeling is that this loosening deserves some positive indication from the developer, one could have the set declaration take a generic:

get id(): string {}
set id<string>(val: string | number) { ... }

this works even if you don't want to define a getter:

set id<string>(val: string | number) { Object.defineProperty(this, 'id', String(val), { configurable: false, writable: false, enumerable: true }) }

@MaximSagan
Copy link

MaximSagan commented Jan 20, 2023

Just spent an hour debugging an error caused by a junior dev that never would have existed if TypeScript could recognize getter-less properties as such. Would be great if this issue were prioritized.

@thw0rted
Copy link

Maxim, I think you're having a slightly different issue. You want this linter rule which prevents the creation of setter-only properties. I think there might actually be an issue on this tracker about it, but under the hood this is basically a problem of bad JS/ES design, and TS is kind of stuck with it.

@nopeless
Copy link

nopeless commented Jan 20, 2023

Just spent an hour debugging an error caused by a junior dev that never would have existed if TypeScript could recognize getter-less properties as such. Would be great if this issue were prioritized.

@MaximSagan
If there is no getter, then the type would be never meaning that it will always extend the setter type. If this issue gets "fixed", then it will actually hide your problem even further. As @thw0rted commented, you want a linter rule.

Or you can just scream at the junior dev for not making it an idiomatic setProperty() function

@thw0rted
Copy link

If there is no getter, then the type would be never meaning that it will always extend the setter type

I think Maxim's point was that if getters and setters were allowed to be incompatible, then the missing getter could be typed, whether explicitly or implicitly, as void (instead of never), and f(foo.writeOnlyProperty) would be an error. This isn't how ES actually behaves, of course, but it would do a better job of observing the Principle Of Least Surprise.

For now, we have that linter rule.

trekladyone pushed a commit to trekladyone/angular that referenced this issue Feb 1, 2023
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close angular#48476
@shellscape
Copy link

A periodic reminder to the TS staff that the community has been asking for this since 2015 (#2521).

@andrewbranch
Copy link
Member

I actually brought this up a few weeks ago and we agreed it’s worth some experimenting during 5.1. I just forgot to update the issue afterwards; thanks for the reminder.

To be fair though, #2521 is closed because #42425 was thought to address it. This issue was raised afterwards to present use cases that #42425 did not address.

@andrewbranch andrewbranch added Experience Enhancement Noncontroversial enhancements and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Feb 27, 2023
@andrewbranch andrewbranch self-assigned this Feb 27, 2023
@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Feb 27, 2023
@andrewbranch andrewbranch added this to the TypeScript 5.1.0 milestone Feb 27, 2023
@travislwatson
Copy link

Perhaps a unique use case, but I'll log mine here. I wanted to allow providing an update function to pass to Immer in addition to just setting the new value. This works fine in JS, but I can't get the Type definitions to comply

console.log(someObj.profile) // { name: 'John Doe' }
someObj.profile = { name: 'This works' }
someObj.profile = (draft) => {
  draft.name = 'This does not'
}

@Morglod
Copy link

Morglod commented Mar 17, 2023

use case:

getter/setter in mapped type for proxy value with different types for reactive (rxjs like) api

I have Proxy object, that simplifies api for rxjs like values
When I get value from this Proxy, I just read last memoized value
When I assign to this value, I should be able to assign both 'updater' or value type

Example code:

values: MyProxy<{ x: number, y: number }>;

values.x = 10;
values.y = () => values.x + 10;

const finalY = values.y;

Type of Proxy here should be:

{
    get [K in keyof T](): T[K];
    set [K in keyof T](value: T[K] | (() => T[K]));
}

Not found any hacks/workarounds here, because mapping object with get/set captures only getter's type:

{
        [k in keyof T]: {
            get x(): T[k];
            set x(val: T[k] | (() => T[k]));
        }["x"];
}

// results in

{
        [k in keyof T]: T[k]
}

Actually typescript should add 'set' modifier for fields along with eg 'readonly'; Which should work same way and check assigment type:

type T;

type T_add_set = {
    set [k in keyof T]: T[k] | (() => T[k])
}

type T_remove_set = {
    -set [k in keyof T_add_set]: T_add_set[k]
}

And 'get' modifier is just 'readonly' which will be resolved similar to function overloading (if 'readonly' and 'set' presents for same field)

@shellscape
Copy link

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.