Don't pollute global namespace

[aslakhellesoy]

It would be nice to be able to do something like this:

(function(stepdefs) {
  // Defines Given, When, Then
  stepdefs.register(this);
  // Or maybe we have to do it like this:
  var Given = When = Then = stepdefs.register;

  var cukes;

  Given(/I have (\d+) cukes in my belly/, function(n, callback) {
    cukes = n;
    callback();
  });

  // More stepdefs here...

})(require('cucumber/stepdefs'));

[aslakhellesoy]

Never mind - this is better:

var Given = When = Then = require('cucumber/api').stepdef;

var cukes;

Given(/I have (\d+) cukes in my belly/, function(n, callback) {
  cukes = n;
  callback();
});

[jbpros]

The global namespace is indeed (temporarily) polluted by Given, When and Then during support code loading. This is definitely wrong. On the other hand, it would be nice if

var Given = When = Then = require('cucumber/api').stepdef;

was not necessary in step definitions "blocks". It would be cool to have those automatically available. But I didn't figure out a way to do that. We'd need to inject the variables into the support code function scope when calling it; something similar to how func.apply(obj) alters this. Is this even possible?

[fernandoacorreia]

How about something like this, inspired in cucumis:

var Cucumber = require('cucumber');
var assert = require('assert');

var Calculator = function() {
    this._stack = [];
};

Calculator.prototype = {
    enter: function (value) {
        this._stack.push(value);
    },

    add: function() {
        this._stack.push(this._stack.pop() + this._stack.pop());
    },

    subtract: function() {
        this._stack.push(-(this._stack.pop() - this._stack.pop()));
    },

    result: function() {
        return this._stack[this._stack.length - 1];
    },
};

var calc;

Cucumber.Given(/^I have a calculator$/, function(callback) {
    calc = new Calculator();
    setTimeout(function() {
        callback();
    }, 10);
});

Cucumber.Given(/^I have entered (\d+) into the calculator$/, function (value, callback) {
    calc.enter(parseInt(value));
    callback();
});

Cucumber.When(/^I press add$/, function (callback) {
    calc.add();
    callback();
});

Cucumber.Then(/^the result should be (\d+) on the screen$/, function (value, callback) {
    assert.equal(calc.result(), parseInt(value));
    callback();
});

Cucumber.export(module);

[jbpros]

I think I prefer calls to Given(), When() and Then() instead of Cucumber.Given(), etc. It's more concise, less noisy and thus reads better.

On the other hand, supplying the step definition method as require('cucumber').stepdef has one drawback: it does not enforce people to use Given, When and Then. But is it really a problem to give responsibilities to people? :)

[fernandoacorreia]

We'd need to inject the variables into the support code function scope when calling it;
something similar to how func.apply(obj) alters this. Is this even possible?

I don't think that's possible. JavaScript uses lexical scoping. The scope of the references is static and it's evaluated at parse time. The only way around would be to convert the function to text, modify the text and reparse it, which is a bad idea on many levels.

Given this restriction, I can think of two options that avoid the need to qualify the call to Given, When and Then, namely having these identifiers either in the global or in the module scope:

1. Leave the functions in the global scope. This would require the less code but is a bad practice and can lead to hard-to-find bugs.

var steps = function () {
    Given(/expression/, function(n, callback) { }
};
module.exports = steps;

2. Declare the methods in the module. This is explicit, if a bit peculiar.

var Given = When = Then = require('cucumber/steps');

var steps = function () {
    Given(/expression/, function(n, callback) { }
};
module.exports = steps;

3. A different approach would be to call the step definition function using call or apply inside the context of an object that defines Given, When and Then. At runtime, "this" in the function would be an object set by cucumber, not the global scope. In this case, it wouldn't be necessary to require the cucumber module but the calls would have to be qualified with "this".

var steps = function () {
    this.Given(/expression/, function(n, callback) { }
};
module.exports = steps;

[jbpros]

I like the 3rd approach but didn't go that road just yet because I was reserving this for a possible way of implementing World.

But it's actually not within the same scope and should work just fine:

var steps = function() {
  // `this` is set to an object supplying Given, When, Then,
  // And, But and maybe more useful stuff.
  this.Given(/exp/, function(callback) { 
    // `this` is set to the World object
    this.myAwesomeHelperDefinedBySomeSupportCode();
    callback();
  });
};
module.exports = steps;

We could even move the step definition callback to a special property of World:

var steps = function() {
  this.Given(/exp/, function() { 
    this.myAwesomeHelperDefinedBySomeSupportCode();
    this.callback();
  });
};
module.exports = steps;

I'm not sure about this one though; maybe it's too implicit. WDYT?

[fernandoacorreia]

I like the idea of using the World instance to define the methods Given, When and Then. And I like the syntax too. I think it fits.

Having World will also make it easier to share state between step definitions.

As for using implicitly a callback property on the World instance, I think it may be a source of confusion. As I understand, the World instance is shared by all step definitions, and its properties are shared as well. It doesn't belong to a single method's callback property to be published in such a visible way.

Any @instance_variable instantiated in a Step Definition will be assigned to the World, and can be accessed from other Step Definitions.

I suppose my Python exposure led me to agree with "Explicit is better than implicit". Magic can save a few keystrokes but at the cost of making it more difficult to grasp what's going on, especially for newcomers.

[jbpros]

I agree. Let's be explicit and keep our callbacks passed as function parameters.

[fernandoacorreia]

Now, rereading, I see what you mean. The World would not contain "Given", "When" and "Then"; that would be in a separate object that would be in "this" when the var steps = function() is called.

When a step definition is called, "this" would point to the World object of the scenario, not to the same object that has Given, etc. I think that's a good idea and I don't think that having a different context for each kind of function is confusing. Especially because World has a very clear definition: its scope is the scenario.

[jbpros]

Yeah you got it right!

And a cool thing is that we can still do the following:

var steps = function() {
  var Given = When = Then = this.Given;
  Given(/exp/, function() { 
    // ...
  });
};
module.exports = steps;

;)

I created a separate issue regarding "World".

[jbpros]

The commit above is a spike and will be deleted soon. It breaks several specs and probably all the features.

It is there to demonstrate and test the new approach we discussed previously. Give it a try by checking out the spike branch and running the following feature:

$ bin/cucumber.js features/no_more_pollution.feature -r features/no_more_pollution_steps.js

Have a look at the step definitions, it's where the shiny things lie ;)

[fernandoacorreia]

Not bad at all. Well done. It's nice to see how the code in library.js got simpler too.

As an aside, in the function buildSupportCodeInitializerFromPaths in support_code_loader.js the name "initializer" is being used for 2 different things in the same function:

var initializer = function() {

and

var initializer = require(path);

Yesterday, when I was browsing the code, I had to look twice to be sure of what initializer was being returned by the function. If you're going to touch that code, consider giving one of the two identifiers a distinct name.

[jbpros]

Thanks for the feedback, Fernando. library.js is indeed cleaner. I'm rewriting it properly (i.e. as BDD) and the spec will be waaaaaaaay cleaner too. This is a good sign ;)

You're right about the var name. It's technically correct but hard to read. Going to rename that.

[jbpros]

Nice side-effect of this API change: step definitions can be added directly to the support code library:

var cucumber = Cucumber(featureSource, supportCode);
var library  = cucumber.getSupportCodeLibrary();
library.defineStep(/^I cuke$/, function(callback) {
  console.log("Cuke!"); callback(); 
})
// ...
cucumber.start(function(succeeded) {
  // ...
});

Can be handy!

[aslakhellesoy]

Getter functions are possible in JavaScript, but not commonly used :-)

var library  = cucumber.supportCode;

sounds more idiomatic to me

[jbpros]

Yep, you're right. I've used the getter approach everywhere, mainly because of the way I spec things. And also because of my previous non-JS background.

Should we change this one, all the other getters should also be removed. I don't think it's the top priority right now but if you think it would be better, we can consider it seriously. This is not really complicated to refactor, just a repetitive task.

[fernandoacorreia]

That's a lot of work you put in. Good job.

As for the current discussion, I'm new to JavaScript myself but I think it would be better if all public APIs were idiomatic, if possible.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.