Yarn: [Code style] The boolean trap

[arcanis]

If you're not familiar with the boolean trap, here is a short summary extracted from the article:

Let’s start with the textbook example: guess what this code means?

widget.repaint(false);

Without looking at the documentation, the usual suspect is don’t repaint the widget. Then, you look at the documentation and it refers the function argument as immediate, which is true if you want immediate painting or false for deferred painting. Thus, the correct behavior implied by the above code is actually repaint this widget later, which is miles away from your initial guess.

We have a few place where we fall into the boolean trap. For example, without looking, can you tell without any doubt the signification of the boolean arguments in the following places?

await setVersion(config, reporter, flags, args, true);
await buildTree(this.resolver, this.linker, patterns, opts, true, true);
await ls(manifest, this.reporter, true);
await this.fork(GitResolver, true, `${sshUrl}#${commit}`);
...

Note that the last one is actually a double trap: this true isn't actually an option, it's an argument that is then passed to the Git resolver. Ideally, these examples could be reworded as such:

await setVersion(config, reporter, flags, args, {isRequired: true});
await buildTree(this.resolver, this.linker, patterns, opts, {onlyFresh: true, ignoreHoisted: true});
await ls(manifest, this.reporter, {isSaved: true});
await this.fork(GitResolver, { forked: true }, `${sshUrl}#${commit}`);
...

It would be a good first step. However, I suggest we go further: let's move into the trailing object all the parameters that have strictly no semantic meaning in the context of the function. For example, consider the following function call:

await setVersion(config, reporter, flags, args, true);

Here, the flags and args parameters have a semantic meaning. We are calling the setVersion command, which takes options and arguments, so it makes sense for them to be first-class arguments. However, everything else (config, reporter and isRequired) is required only because of implementation details and doesn't help to actually understand the code flow.

Because of this, I suggest we write the following instead:

await setVersion(flags, args, {config, reporter, isRequired: true});

The example above would then become:

await setVersion(flags, args, {config, reporter, isRequired: true});
await buildTree(patterns, {resolver: this.resolver, link: this.linker, onlyFresh: true, ignoreHoisted: true, reqDepth: ... });
await ls(manifest, {reporter: this.reporter, isSaved: true});
...

---

Is it required?

No, it's just a stylistic proposal that I think could improve the readability. If you think this is nonsense we can drop it :)

Would we have to rewrite all the methods?

No, it's something that would only affect the new code we write and potentially the existing code we would need to rework anyway while implementing features and/or fixing bugs. There's no need to update everything at once.

Would it work with prettier?

I think so. It would follow the classic conventions: until 80 columns it would stay on the same line, and more than that it would go multiline. I think there is some room of improvement on the prettier/Flow interaction on the object typing (more on that below), but overall there's no blocker.

Would it work with Flow?

There's a bit more work because Flow has a somewhat particular syntax to declare argument objects, partly because of the JS spec preventing them to use the same syntax as everywhere else (because { config: Config } means "assign config to the variable Config in destructuring contexts).

So, instead of writing:

async function setVersion(
  config: Config,
  reporter: Reporter,
  flags: Object,
  args: Array<string>,
  isRequired: ?boolean
) {}

We would write:

async function setVersion(
  flags: Object,
  args: Array<string>,
  {
    config,
    reporter,
    boolean = false
  }: { config: Config, reporter: Reporter, isRequired: boolean }
) {}

I would personally prefer Prettier to mirror the style regardless of the width, so that it would become:

async function setVersion(
  flags: Object,
  args: Array<string>,
  {
    config,
    reporter,
    boolean = false
  }: {
    config: Config,
    reporter: Reporter,
    isRequired: boolean
  }
) {}

But that's a good first step regardless.

---

Opinions?

[arcanis]

cc @bestander @voxsim and anyone else interested into giving their opinion! 😃

[rally25rs]

From a 'clean code' perspective, I like this. In fact I would support just moving all the parameters to a single object; make functions have an arity of 1. Especially since ES6 shorthand property names eliminate having to duplicate the object key and value when they match. The additional benefit is that you don't need to remember the argument order any more either ("was it _flags_ then _args_, or _args_ then _flags_?")

await setVersion({flags, args, config, reporter, isRequired: true});

I would only give pause to this change due to Flow. The beauty of JS is not having to actually define an object definition for this kind of thing. If the additional work to make Flow like it is minimal, then I like this change.

[bestander]

+💯 on getting rid of it

[voxsim]

That's awesome!!! I totally agree :) I thought about implementing an eslint plugin to switch on in some cases and see what it says :P (like someone did here https://github.com/eslint/eslint/pull/7713).

[arcanis]

Then let's try to do this during the next months 👍

@voxsim if you want to make an eslint plugin and are looking for a home to host it, I have an eslint plugin repository that would gladly accept a PR! - but if you prefer to do it on your own repo, I have no issue with this 👐

@rally25rs I'm not completely sold on moving all parameters to a single object - it makes it easier to write code, since as you say there's no issue of argument order, but I feel like it also makes it harder to follow the code flow, since you lose the information of which arguments are really important for the function (which means that you have to assume that all arguments are important).

[BYK]

For anyone interested in doing this: just find or write an ESLint rule that checks function arguments and warns if they are boolean so we know when we are adding new calls.