The Wayback Machine - https://web.archive.org/web/20200529214344/https://github.com/airbnb/javascript/issues/1812
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

Add `err` and/or `opts` to the no-param-reassign exclusions? #1812

Open
tunnckoCore opened this issue May 17, 2018 · 5 comments
Open

Add `err` and/or `opts` to the no-param-reassign exclusions? #1812

tunnckoCore opened this issue May 17, 2018 · 5 comments

Comments

@tunnckoCore
Copy link

@tunnckoCore tunnckoCore commented May 17, 2018

Hey there.

Most frequent i have problem with this rule. And specifically in cases of options, opts and err. I also seen in #1089 that this rules is disabled for different reasons. What's your thoughts? Could you consider adding opts and err?

I know that there is a bit better and more proper way for adding to the error instance, for example creating whole new error class based on the original one. But there is simple things like adding some single metadata property.

In my case such metadata is err.commandArgv and err.commandName where commandName isn't exactly the terminal/unix one, but command of some cli - for example mycli hello so err.commandName will be hello regardless what that command does.

@tunnckoCore tunnckoCore changed the title Add `err` and/or `opts`, `ctx` to the no-param-reassign exclusions? Add `err` and/or `opts` to the no-param-reassign exclusions? May 17, 2018
@tunnckoCore
Copy link
Author

@tunnckoCore tunnckoCore commented May 17, 2018

Actually, for the opts/options that's isn't so big problem, because of using Object.assign. Example

function foo(opts) {
  opts = Object.assign({ bar: true }, opts)
}

Mostly, to workaround that i use both options and opts names like that:

function foo(options) {
  const opts = Object.assign({ bar: true }, options)
}

So at least add err to exclusions?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented May 17, 2018

Options should never be mutated, since other things might be using that object.

Can you explain why you’d need to mutate or reassign err?

@tunnckoCore
Copy link
Author

@tunnckoCore tunnckoCore commented May 17, 2018

Yea, agree for the options.

As about the error. I'm not reassigning it as a whole, i'm adding a bit more info to it and throwing it again. I explained why i want to add property like commandName, so for such reasons. It sounds a waste of time and code to create whole new error class, that extends Error, for one or two new properties.

const app = {
    /**
   * Start the magic. Parse input commands and flags,
   * give them the corresponding command and its action function.
   *
   * @returns {Promise}
   * @public
   * @async
   */
  async listen() {
    const result = prog.parse(process.argv, opts);

    const { args, name, handler } = result;

    try {
      return handler(...args);
    } catch (err) {
      err.commandArgv = args[args.length - 1]; // last one is process.argv
      err.commandArgs = args;
      err.commandName = name;
      throw err;
    }
  },
}

and in the CLI

listen()
  .then(() => {
    // this is CLI file.
    // eslint-disable-next-line unicorn/no-process-exit
    process.exit(0);
  })
  .catch((err) => {
    console.error('Error task:', err.commandName);

    if (err.commandArgv && !err.commandArgv['show-stack']) {
      console.error('Error message:', err.name, err.message);
    } else {
      console.error('Error stack:', err.stack);
    }

    // this is CLI file.
    // eslint-disable-next-line unicorn/no-process-exit
    process.exit(1);
  });
@ljharb
Copy link
Collaborator

@ljharb ljharb commented May 17, 2018

Do you actually need the stack trace from the original error? If not, it seems like it'd be better to make a new Error instance; either way, you can do Object.assign(err, { comandName: 'blah' }) and the linter rule won't complain.

@tunnckoCore
Copy link
Author

@tunnckoCore tunnckoCore commented May 17, 2018

Do you actually need the stack trace from the original error?

Exposing that to third party, so it depends.

and the linter rule won't complain.

Ahhh, sweet! How i forgot about that? 😸 Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.