Skip to content

Support for key in typescript interfaces #1790

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

Closed
AndreasArvidsson opened this issue Aug 15, 2023 · 20 comments · Fixed by #1873
Closed

Support for key in typescript interfaces #1790

AndreasArvidsson opened this issue Aug 15, 2023 · 20 comments · Fixed by #1873
Assignees
Labels
lang-typescript TypeScript/JavaScript grammar support

Comments

@AndreasArvidsson
Copy link
Member

interface Hello {
    value: number
}

value here can be referred to as name, but not key. I would like to support both.
@pokey what do you think?

@pokey
Copy link
Member

pokey commented Aug 15, 2023

Hmm. That kind of opens the door for number to be "value", which we don't want, because that conflicts with the "value" in aaa: number = 0

I can see the benefit, though. Not sure

@AndreasArvidsson
Copy link
Member Author

It's just so annoying when something looks exactly like a key value pair and the obvious thing doesn't work.

@pokey
Copy link
Member

pokey commented Aug 15, 2023

I know what you mean. I wouldn't be opposed. Presumably you're not proposing "value" for the type tho? Although I think sometimes I find myself wanting that too 😅

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Aug 15, 2023

I would want both key and value.

value: number = 0 is not valid syntax when I try it? Are you talking about fields in a class? In that case it's really redundant to give both a type and a default value. If you just give a default valued type will be inferred from that. I would still argue for value taking the type in a interface at least. Maybe we could detect if there is a default value and do something different but I'm not sure it's worth it.

@pokey
Copy link
Member

pokey commented Aug 15, 2023

yeah for classes. and you definitely sometimes give both, eg if you want to ensure you don't miss a field on an object literal or something

yeah might be tempted to detect if there is default value and let that win if it exists

@AndreasArvidsson
Copy link
Member Author

I would start with simple key and value and see how it feels. Number of fields with type and value can't be that many. Not being able to use key and value is any interface feels worse.

@auscompgeek
Copy link
Member

What happens with an index signature?

interface Foo {
  [index: number]: string;
}

@auscompgeek auscompgeek added the lang-typescript TypeScript/JavaScript grammar support label Aug 15, 2023
@pokey
Copy link
Member

pokey commented Aug 15, 2023

Number of fields with type and value can't be that many

It actually appears to be extremely common in our code base. I did a very quick search for class and 4/5 classes I saw had it. But I think the behaviour of preferring the default value when you see both seems reasonable. I think that should end up being fairly intuitive

@pokey
Copy link
Member

pokey commented Aug 15, 2023

What happens with an index signature?

interface Foo {
  [index: number]: string;
}

I think we'd want nested scopes there:

     interface Foo {
       [index: number]: string;
//!1    ^^^^^
//!1    -------------
//!2   ^^^^^^^^^^^^^^^
//!2   ------------------------
     }

@AndreasArvidsson
Copy link
Member Author

Number of fields with type and value can't be that many

It actually appears to be extremely common in our code base. I did a very quick search for class and 4/5 classes I saw had it. But I think the behaviour of preferring the default value when you see both seems reasonable. I think that should end up being fairly intuitive

Well we mostly use classes for targets and there we have a common base. I don't know if I ever used both type and default value outside of cursorless.
Yes but don't know ow how messy the scm might end up.

@auscompgeek
Copy link
Member

At my work default values for class fields is a common pattern.

@AndreasArvidsson
Copy link
Member Author

Of course, but that is not what I said. Field with type and default value is unnecessary since the type is derived from the default value.

@AndreasArvidsson
Copy link
Member Author

Probably should just do this for interface and skip class fields. For we have the same problem with const value: number = 0. That mirrors the class fields better.

@auscompgeek
Copy link
Member

Last question from me: how about type aliases and object types?

type Foo = {
  bar: string,
};

function baz(opts: {
  bang: number,
}) {
  let whyWouldYouDoThis: {
    bloop: Foo,
  };
}

const destructure = ({ quux, isFoo }: {
  quux: string,
  isFoo: boolean,
}) => {}

@pokey
Copy link
Member

pokey commented Aug 15, 2023

Probably should just do this for interface and skip class fields. For we have the same problem with const value: number = 0. That mirrors the class fields better.

That's reasonable, tho I might argue we should just apply the same logic there and allow "value" for "type" when there's no value. But I'm fine starting with just disallowing it for class fields and seeing how annoying it is

@pokey
Copy link
Member

pokey commented Aug 15, 2023

Of course, but that is not what I said. Field with type and default value is unnecessary since the type is derived from the default value.

It is sometimes necessary, because typescript can't always infer exactly what type you want from the value itself

@pokey
Copy link
Member

pokey commented Aug 15, 2023

Last question from me: how about type aliases and object types?

type Foo = {
  bar: string,
};

function baz(opts: {
  bang: number,
}) {
  let whyWouldYouDoThis: {
    bloop: Foo,
  };
}

const destructure = ({ quux, isFoo }: {
  quux: string,
  isFoo: boolean,
}) => {}

I'd say those should get the same treatment as interfaces

@AndreasArvidsson
Copy link
Member Author

Of course, but that is not what I said. Field with type and default value is unnecessary since the type is derived from the default value.

It is sometimes necessary, because typescript can't always infer exactly what type you want from the value itself

I know that's why I said that it can't be that many; it is used but it's not too common. I think we should try to optimize for the common use cases with Cursorless. If you make things too complicated it will be hard to remember the rules. Personally I wonder if we should introduce scopes that are just left and right without these kind of nuances.

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Aug 15, 2023

I'd say those should get the same treatment as interfaces

agree

@pokey
Copy link
Member

pokey commented Aug 15, 2023

Either way I think for now starting by leaving out classes is probably ok. I think the place where it blurs a bit is abstract classes, which can feel more like an interface, but we can see how annoying the inconsistency is

Fwiw tho I don't think it will be too hard to implement the heuristic of letting default value take precedence for "value". If the default value is marked by a field you can just use -someFieldName

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-typescript TypeScript/JavaScript grammar support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants