clean-code-javascript icon indicating copy to clipboard operation
clean-code-javascript copied to clipboard

Function Arguments flaw in Typescript implementations

Open bmcminn opened this issue 5 years ago • 3 comments

I'm pointing out a "gotcha" in relation to @ryanmcdermott's comment at the end of #199 in regards to objects as function argument configs.

I agree wholeheartedly with points 1-3 and they have greatly improved some of the readability and usability of some of my personal projects. However, I build an Angular6/Typescript project at work and recently had an opportunity to leverage this approach and here's what I found.

Three years since that comment has apparently yielded some concrete ammunition against your thesis on this approach in virtually all situations in Typescript.

function createSquare({ width, color }) {
  let width = width || 0
  let color = color || 'white'

  // ...
}

let newSquare = createSquare({ width: 123 })

This will cause Typescript to throw a compilation error: error TS2345: Argument of type '{ width: 123 }' is not assignable to parameter of type '{ width: any; color: any; }'. as this appears to violate Typescripts deterministic validation of your code.

The work around to this in Typescript requires at least an interface of the methods argument object signature. As a JS purist, I hate this because it obfuscates your method signature away from your implementation in the code, but if you're using a sophisticated enough IDE then technically intellisense/autocomplete would handle the issue for you and DocBlock comments could easily handle explaining the situation, but it just feels bad to me :P

// excerpted from https://www.typescriptlang.org/docs/handbook/interfaces.html

interface SquareConfig {
  width: number;
  color?: string;
}

function createSquare(params: SquareConfig) {
  let { width, color } = params

  // ...
}

let newSquare = createSquarey({ width: 123, color: 'blue' })

It's still not great in my opinion compared to the inherit flexibility in native JS, but something I thought worth mentioning/discussing because this apparently is a hell of an exception to the rule.

Cheers!

bmcminn avatar Jan 10 '20 21:01 bmcminn

Wouldn't it be much simpler like this?

type SquareConfig {
  width: number;
  color?: string;
}

function createSquare({ width = 0, color 'white' }: SquareConfig) {
   // ...
}

let newSquare = createSquarey({ width: 123, color: 'blue' })

I've been using this for a long time and as long as you're coding in a decent IDE, it's working like a charm.

As for being a JS purist, I was one too. I understand it's weird to define a Type or an Interface and the whole idea of Typescript had me thinking "is this really necessary, just code it right, it's not that hard...".

But after actually using Typescript with Types and Interfaces, I now can't live without it. To the point of I'm not even considering pure JS jobs anymore...

Typescript does have its rough edges and you'll bang your head on the wall a lot when dealing with very complex types... But overall, it's such a huge net gain! It's like suddenly not having to do 3 hours of commute every day, you have so much more time to do stuff you actually wanna do, it's surreal...

TheThirdRace avatar Oct 04 '21 02:10 TheThirdRace

@TheThirdRace until you want to do something as trivial as parse a list of serialized key/value pairs into an object and have spend an hour or so googling around for an answer only to find out you have to go out of your way to explicitly designate that your "custom" options object uses string based indexes because TS is too safe to assume anything...

function doSomething(...args) {
    // we have to do this because TS has no idea how to resolve what
    //  property you're trying to access otherwise...
    const options: {[index: string]: any} = {}

    args.forEach(arg => {
      const [key, value] = arg.split('=')
      options[key] = value
    })
    // ...
}

// error TS7053: Element implicitly has an 'any' type because expression of type 'any' can't be used to index type '{}'.
// LINENO ## options[key] = value

Personally the number of times TS has wasted an hour or two of my time because I had to search for the one bit of esoteric information I need to correct the problem instead of the compiler giving me a resource link to reference like other CLI tools do has continually soured my opinion of it. It's the same kind of issues I had with Coffeescript, the benefits usually don't outweigh the cost associated with adopting it into my workflow.

bmcminn avatar Oct 05 '21 00:10 bmcminn

Well, I certainly get that JS flexibility is hard to let go. Been there, done that...

My point is more about how much flexibility you want. My experience has been that given too much rope, 95% of developers will hang themselves with it... The bugs multiply and they happen at runtime, which is the worst kind of bug.

Typescript forces you to follow some rules and the bugs happen while coding or at build time. You pretty much squash any runtime bugs before they ever reach production.

Typescript reporting of problems and errors are often not written for normal people, I will 100% agree with you on that. It's very infuriating sometimes and there's definitely a learning curve! But spending 1 or 2 hours figuring it out is much less impactful than spending 2 days in PROD trying to debug something that could easily have been prevented with basic typings. And lets not forget about the lost data that could incur, the bad user experience and the whole pile of troubles arising from runtime errors.

As for your example, I think it's perfectly normal, and wanted, that Typescript is giving you trouble in this case.

options should have a well defined type, you already know which options your function supports so there's no reason whatsoever to allow anything else. That's why you'll very rarely use ...args in a function nowadays. You will instead receive a typed object and this will eliminate all risks of errors related to wrong or deprecated options.

Also, parsing a string is not really reliable. It's a very loose coupling between your function and its arguments. A change in the code calling your function is very likely to destabilize the whole process. Not only you won't be alerted of this new error ahead of time, but you'll discover the error at runtime, sometimes relatively long after being in production. In fact, I can guarantee you 100% there will be runtime bugs if you're working with anyone that isn't in the top 5% JS developers. Furthermore, parsing has the bad habit of introducing security flaws into your application.

I think you're simply not using Typescript with the right mindset. Both examples that you gave were more or less about the technique used than anything else.

Don't get me wrong, I know your examples work in JS, they will run perfectly fine if you're doing your job well. But at the same time, in a static typing mindset, you're totally screwing yourself...

JS is wonderful for its flexibility and fast coding pace, but the ugly part comes at runtime when bugs can be particularly hard to find and solve. This is where Typescript shines, you trade a bit of convenience at build time to reap huge benefits on the long term at runtime. And let me assure you, you're in much much much better position when your code fails at build time than runtime...

I hope it clarifies a bit more the point I was trying to make, which is to approach the problem with a different mindset.

Have a nice day!

TheThirdRace avatar Oct 05 '21 02:10 TheThirdRace