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

Do not modify input parameters #504

Closed
wants to merge 1 commit into from
Closed

Do not modify input parameters #504

wants to merge 1 commit into from

Conversation

jcstanaway
Copy link

This should resolve issue #503. The test app from #503 works.

@AVVS
Copy link
Collaborator

AVVS commented Jul 25, 2017

Change as it is will work, but, at the same time, there will be a performance penalty associated with this, as we'd have to create shallow copies of args for each command

@luin what do you think?

@jcstanaway
Copy link
Author

It appears that this PR isn't needed. There is another commit on (June 8th) for #480 which also appears to resolve the issue and would probably explain why @luin wasn't able to reproduce it: I'm using the released 3.1.1 and @luin was likely running off of the latest code. 3.1.1 does not include the commit for #480.

If that's the case, then this PR can be rejected.

3.1.1 code:

Pipeline.prototype.addBatch = function (commands) {
  for (var i = 0; i < commands.length; ++i) {
    var command = commands[i];
    var commandName = command.shift();
    this[commandName].apply(this, command);
  }

  return this;
};

Latest version:

Pipeline.prototype.addBatch = function (commands) {
  var command, commandName, args;
  for (var i = 0; i < commands.length; ++i) {
    command = commands[i];
    commandName = command[0];
    args = command.slice(1);
    this[commandName].apply(this, args);
  }

  return this;
};

@luin
Copy link
Collaborator

luin commented Jul 26, 2017

@ccs018 Thank you for pointing this out! Just released v3.1.2. Sorry for not releasing this version when the fixed had been made.

@luin luin closed this Jul 26, 2017
@jcstanaway
Copy link
Author

Thanks for the updated release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants