Clone command options before passing into commands

[lilaconlee]

Current behavior:

When commands receive options, it is the original object and not a clone. This has resulted in a few bugs (#2707, #365), and probably some more I couldn't quickly track down.

Desired behavior:

This should be done before options are passed into commands so all commands receive options in the same frozen state.

[flotwig]

I don't think this is a good idea, because cloning every single argument sent to every single command unnecessarily is pretty wasteful and will add some slowdown to Cypress, especially if people are using large objects in commands.

I think that commands that use the identity of the object, or modify the object passed in, should clone options themselves as needed. We should probably do some kind of audit/add new tests to catch others like cy.setCookie that don't act right.

[bahmutov]

We could at least freeze a command object to avoid messing with it

…

Sent from my iPhone

On Nov 7, 2019, at 16:05, Zach Bloomquist ***@***.***> wrote:  I don't think this is a good idea, because cloning every single argument sent to every single command unnecessarily is pretty wasteful and will add some slowdown to Cypress, especially if people are using large objects in commands. I think that commands that use the identity of the object, or modify the object passed in, should clone options themselves as needed. We should probably do some kind of audit/add new tests to catch others like cy.setCookie that don't act right. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[flotwig]

I tried a little thing with Object.freeze and using use strict to make it err when you modify a property:

function callCommand(cmdFn, args) {
  'use strict'
  Object.freeze(args)
  try {
    cmdFn(args)
  } catch (e) {
    console.log('The options passed in were modified')
    console.log(e)
  }
}

callCommand(
  (args) => { args.b = 'foo' },
  { b: 'bar' }
)

...but since the callback is defined outside the use strict, this doesn't error unless you inline cmdFn, so just doing Object.freeze isn't enough to catch these issues.

I think if we just freeze random objects without visibility, it's likely to cause more bugs than it would solve.

---

Maybe we could use Proxy to create something like Object.freeze that actually errors when you modify a property on the passed-in object, then recursively apply this to command arguments when CYPRESS_ENV !== production or something.

[sainthkh]

@flotwig @bahmutov

The cause of this problem is the line like this in each command:

_.defaults options,
  $el: subject
  log: true
  force: false

(This line is from check.coffee)

As you know, _.default() mutates the object (in our case, options).

Because of this, freezing will just make problem bigger. (You cannot add default values to the options object.)

To solve this problem, we need to open every command file and fix all those lines like this (fix 1).

opts = _.default {}, options, 
  $el: subject
  log: true
  force: false

And change options with opts.

Or we can write like this (fix 2):

options = _.defaults {}, options, 
  $el: subject
  log: true
  force: false

In this case, you don't have to change options.

Personally, I prefer the first solution because it doesn't override given argument value.

As a matter of fact, I have a related PR (#5762). I had to open every command to show user-defined options value.

When I was writing it, I didn't know about this issue and thought options mutation was part of intended feature.

If you let me, I will revisit that PR and fix those _.default() with fix 1 or fix 2.

[flotwig]

@sainthkh A PR to get rid of the _.defaults mutations would be welcome. You're right, this is unintended behavior, we don't want to modify what the user passes in at all.

[cypress-bot]

The code for this is done in cypress-io/cypress#6459, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

[cypress-bot]

Released in 4.4.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v4.4.0, please open a new issue.