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

Refactoring to convert to "named parameters" #23552

Closed
DanielRosenwasser opened this issue Apr 19, 2018 · 28 comments · Fixed by #30089
Closed

Refactoring to convert to "named parameters" #23552

DanielRosenwasser opened this issue Apr 19, 2018 · 28 comments · Fixed by #30089
Labels
Committed The team has roadmapped this issue Domain: Refactorings e.g. extract to constant or function, rename symbol Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

It would look something like this

converttonamedparameters

@DanielRosenwasser DanielRosenwasser added Domain: Refactorings e.g. extract to constant or function, rename symbol Suggestion An idea for TypeScript labels Apr 19, 2018
@DanielRosenwasser
Copy link
Member Author

CC @rauschma @appsforartists in case you want to 👍

@DanielRosenwasser DanielRosenwasser changed the title Refactoring to convert to named parameters Refactoring to convert to "named parameters" Apr 19, 2018
@rauschma
Copy link

rauschma commented Apr 20, 2018

Apologies for being ungrateful: this doesn’t reflect how I use this pattern – I don’t see the parameters as a whole, I see each parameter individually. As a work-around, I inline the type and don’t define it externally.

Let me try to convince you, one last time (then I’ll stop pestering you), of the usefulness of a better notation for destructuring (nested destructuring will profit, too!). The following is an extreme example, but there are many similar functions in my code (“Showoff” is the name of my slide framework):

function slidesToNodeTree(
  {conf, configShowoff, inputDir, slideDeckDir, slideFileName, parentPartNode, visitedSlideFiles}
  : {conf: ConfigSlideLink, configShowoff: ConfigShowoff, inputDir: string,
    slideDeckDir: ServePath, slideFileName: string, parentPartNode: PartNode,
    visitedSlideFiles: SlideFileDesc[]}) {
  ···
}

With a better notation:

function slidesToNodeTree(
  { conf as ConfigSlideLink, configShowoff as ConfigShowoff, inputDir as string,
    slideDeckDir as ServePath, slideFileName as string, parentPartNode as PartNode,
    visitedSlideFiles as SlideFileDesc[]}) {
  ···
}

Alternatively:

function slidesToNodeTree(
  { (conf: ConfigSlideLink), (configShowoff: ConfigShowoff), (inputDir: string),
    (slideDeckDir: ServePath), (slideFileName: string), (parentPartNode: PartNode),
    (visitedSlideFiles: SlideFileDesc[])}) {
  ···
}

Note how, in the last two examples, you actually see parameters with names (vs. a single type for all parameters). The first example looks messy, the last two examples don’t.

If you have ever used a programming language with named parameters and liked them there – isn’t this a compelling use case? Given the thumbs-up at a recent comment of mine, there are quite a few people who agree.

This is the only aspect of my plain JavaScript code that became less usable after I moved to TypeScript.

@Kingwl
Copy link
Contributor

Kingwl commented Apr 20, 2018

a useful feature 👍

@appsforartists
Copy link

I love that you're exploring this, but I agree with @rauschma.

I think there's a bit of a cargo-cult bias in the TS community to always prefer interfaces (because they can be extended). Type literals seem like a more appropriate model for the kwargs pattern, because they allow users to treat each named argument individually.

If a third party made a "convert to named arguments" command, I would probably find it useful to be able to write function signatures normally and then convert them to an interface/type literal. However, I'd rather the language supported setting the type for a named arg adjacent to the declaration of its name and default value. It's both hard to read and cumbersome to maintain when the types are separated from the rest of the definition.

@mhegazy mhegazy added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Apr 20, 2018
@cancerberoSgx
Copy link

This refactor also must refactor all references to the method in the whole project right ? But a very helpful refactor , in my experience, happened many times when an API signature that needs to be backwards compatible, is defined with multiple params and then you keep adding parameters to implement new features or even change parameter type to OR, like existing: PreviousType|NewSemanticType....

@Kingwl
Copy link
Contributor

Kingwl commented Jun 19, 2018

This refactor also must refactor all references to the method in the whole project right ?

IMO, it should

@cancerberoSgx
Copy link

So I was playing a lot lately with Language Service APIs and friends and I have several refactors more or less working fine. This is the one suggested here (I think) :

https://github.com/cancerberoSgx/typescript-plugins-of-mine/tree/master/typescript-plugin-proactive-code-fixes#transform-parameter-list-into-single-object-parameter

You can easily install them in vscode as an extension: https://marketplace.visualstudio.com/items?itemName=cancerberosgx.vscode-typescript-refactors

Probably TypeScript team will implement these more elegantly but in the meanwhile at least is fun. This is kind of a crazy tool that was really helpful to develop plugins quickly and learn the API: https://github.com/cancerberoSgx/typescript-plugins-of-mine/blob/master/typescript-plugin-ast-inspector/doc/evalCodeTutorial.md

And now I'm playing with some IPC communication between plugins in tsserver and host editor plugins /
extensions / packages that provide generic input-related operations - so I can inquire user for data in my refactors visually and keep being editor / IDE agnostic. For example, if I want to move a method to another class, or to another file I need to ask the user the destination and for that plugins can talk with these "Input Providers" generically. Since I'm manipulating the SourceFiles myself (not using FileEditRange ) I can do it synchronously. Really enjoying it will update you when I have something pretty to show.

@mohsen1
Copy link
Contributor

mohsen1 commented Dec 5, 2018

This is great!

Have you thought about the default arguments with no types? Would you infer types there?

function foo(a = 1, b?: string) 

Also would it work in constructor function with private or public keyword? I think it won't, right?

class Foo {
  constructor(private a: string) {}
}

where would you generate a type name if function is anonymous?

(function (a = 1, b?: string){}).call(1)

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Dec 14, 2018

This would be super helpful to me, even if others would rather a new destructuring syntax for function parameters.

So, if the proposed refactoring somehow loses favour over new destructuring syntax, I would still like for this particular one to be implemented.

I personally use interfaces instead of inlining object parameter types, especially for large projects. And projects I write that get consumed by others.

I've had to wrap a function many times. And if the library doesn't have those function arguments as an interface, then I have to copy-paste their inlined code instead of just using an interface that should already be there.

People like to think of interfaces as being "code duplication". But in the big picture, not having those interfaces causes code duplication.


So, in my personal opinion, inlining = save time now, but cause code duplication in the future/for downstream users of your library. Interface = a little extra time now, but reduce code duplication and make it easier for downstream users to extend/reuse/wrap code.

@DanielRosenwasser
Copy link
Member Author

@mohsen1 you're now my favorite QA contact for refactorings 😉

Have you thought about the default arguments with no types? Would you infer types there?

Good test case! Should "fall out" from the naive implementation.

Also would it work in constructor function with private or public keyword? I think it won't, right?

It probably should not for now.

where would you generate a type name if function is anonymous?

As a first-pass, it's probably reasonable to say that this would only generate a named type for

  1. Function declarations
  2. Single-variable declarations initialized with some sort of function expression/arrow function
  3. Constructors without parameter properties
  4. Method declarations

We could always come back to this and add it for signatures like call type literals, construct type literals, call/construct signatures, and method signatures (i.e. the ambient stuff).

@Kingwl
Copy link
Contributor

Kingwl commented Jan 10, 2019

ummmmmm, I'd like to work on it (If I didn't disrupt your release plan)

@zpdDG4gta8XKpMCd
Copy link

Options your say, why not Params?

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jan 10, 2019

Params is fine too - the important thing is that the refactoring provides a rename location on the generated type so that editors can immediately start a rename session.

@Kingwl this one might involve more work than other refactorings, but nobody's started working on it yet, so go for it!

@ghost
Copy link

ghost commented Jan 14, 2019

@rauschma then you have two syntaxes to be maintained. Because your version is not compatible with the current version that allows sharing of interfaces between methods. So you get an inconsistency there, because you can't just abolish the current syntax because of exactly this reason.

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Jan 14, 2019
@RyanCavanaugh
Copy link
Member

@Kingwl we've decided to have our intern @gabritto implement this for her project - apologies if you've already started on it

@Kingwl
Copy link
Contributor

Kingwl commented Jan 15, 2019

@RyanCavanaugh
Please feel free, Actually, I haven't started that work

@DanielRosenwasser
Copy link
Member Author

For the record, I'd like to not continue discussion around new syntax here. This feature could theoretically work with that syntax anyway so it shouldn't have any bearing on the implementation.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 5, 2019

✨ 🚲 🏠 ✨

Naming poll! Vote:

  • 🎉 "Convert to named parameters"
  • ❤️ "Convert to parameters object"
  • 🚀 "Convert parameters to destructured object"
  • 👎 Other (specify)

@OliverJAsh
Copy link
Contributor

I call them named parameters, but just throwing another idea out there: "convert parameters to destructed object"

@ShawnTalbert
Copy link

I wish we wouldn't encourage the pattern of making functions take a single object - it thwarts currying.

@tjpalmer
Copy link

tjpalmer commented Mar 9, 2019

I know @rauschma said he'd stop and seems to have kept his word, but he's absolutely right that doesn't express what people want to express.

Have people discussed using is yet? Example from the top using this syntax follows:

function foo({a is number, b is string, c? is boolean}): void {
  a; b; c
}

And the TypeScript code base itself is chock full of long lists of parameters with optionals etc. Empirical evidence for where this could help.

@brasten
Copy link

brasten commented Sep 9, 2019

Have people discussed using is yet? Example from the top using this syntax follows:

function foo({a is number, b is string, c? is boolean}): void {
  a; b; c
}

There were some discussions about as in an earlier issue -- mostly involving concerns about introducing a new keyword in a very specific context. I suspect is would have the same complaints.

I haven't thought through every issue, but I'm wondering about using : twice, where the renamed variable name can be optional. So instead of [object_key]: [variable_name] = [default_value] you end up with [object_key]: [variable_name]: [type] = [default_value]. This kind of makes sense since you're typing the variable name, rather than the object key, anyway.

So the most verbose version of this would be:

function someFunc({ foo: bar: string = 'default' }): void {
  bar;
}

... whereas the common case would be:

function someFunc({ foo:: string }): void {
  foo;
}

@AnyhowStep
Copy link
Contributor

I wish we wouldn't encourage the pattern of making functions take a single object - it thwarts currying.

There are cases where it is beneficial to take a single object with many parameters.

For example, that object can be an immutable data structure and the function is just performing some operation on it and returning a new instance of the data structure.

In some cases, those data structures can have dozens of properties... You're not really suggesting we curry in those cases as well, right?

@jasonwilliams
Copy link

@DanielRosenwasser what's the reason this was closed?
I was looking for this feature earlier and came across this issue.

@RyanCavanaugh
Copy link
Member

@jasonwilliams it was closed because it was implemented

@uglycoyote
Copy link

uglycoyote commented Jan 5, 2022

I love this refactoring, but is there a a reason why it does not work on constructors?

I sometimes find myself creating a class where the number of constructor parameters grows out of control, with too many optional ones and I wished that I had instead made one "config" or "options" interface that can be passed in to the constructor. The advantages being:

  1. the constructor calls would be clearer, with objects passed in which explicitly have {key:value, key:value} rather than just a bunch of unnamed values passed in as constructor parameters, and
  2. it makes it easier/cleaner to deal with a lot of optional parameters, since they can simply be omitted from the configuration object instead of all of the constructor parameters needing to be passed in the correct order (avoiding the awkward situation where you need to pass a dummy value to the 4th optional parameter so that you can a desired value to a 5th optional parameter)

(I guess these are the same reasons why it's useful on functions, really)

I notice that @mohsen1 asked about using it on constructors earlier in this thread and @DanielRosenwasser kiboshed the idea unceremoniously (simply saying "It probably should not for now."). Maybe that's just because it was early on in the development of this refactoring and you all were trying to keep things as simple as possible? What extra complications do using this for constructors create that aren't present for functions?

Since this issue is old and closed should I open a new issue called "Make 'Convert parameters to destructured object' refactoring work for constructors"?

@RyanCavanaugh
Copy link
Member

Yes, please open a new issue. Thanks!

@marco2216
Copy link

If anyone tried this and wonder why it doesn't work, it might be because you are selecting multiple lines.
Placing the cursor on just one parameter in a function and then opening the refactor menu should do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Domain: Refactorings e.g. extract to constant or function, rename symbol Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.