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

Feature request: UI that does not use the this keyword #745

Closed
osher opened this issue Jan 31, 2017 · 17 comments
Closed

Feature request: UI that does not use the this keyword #745

osher opened this issue Jan 31, 2017 · 17 comments
Labels
⚡ enhancement Request for new functionality

Comments

@osher
Copy link

osher commented Jan 31, 2017

Given a world such as:

require('cucumber').defineSupportCode( ({setWorldConstructor:world}) => {
    world(class { 
      constructor(p) { ... }
      foo(bar) { return new Promise(.... ) }
    })
})

I want to be able to write:

  When(/^I do the foo with (.*)$/i, (ctx, bar) => ctx.foo(bar) );

instead of

  When(/^I do the foo with (.*)$/i, function(bar) {
      return this.foo(bar)
  });

This will also rid me from monkeying the .bind(this) on all async operations, or if you prefer - rid of monkeying with var that = this.

One more benefit (IMHO)- it will help people get off inheritance tree for worlds and orient them better towards the better powers of JS

@osher
Copy link
Author

osher commented Jan 31, 2017

I agree that it will mean you always accept world as argument, and that's an API change.
I think that like mocha wich has --ui exports and --ui tdd and --ui bdd - the cucue can as well.
It's basically a project variable that determines if steps are called/applied on the world, or accept it as 1st argument.

@charlierudolph
Copy link
Member

I really don't think this would be worth it in the long run. Yes its nice for those 1 line steps where you get to use the => where you otherwise be unable to. In my experience I've had very few steps like this. I'm surprised ES6 didn't include an arrow function that does not retain context as that would be ideal to me.

This will also rid me from monkeying the .bind(this) on all async operations, or if you prefer - rid of monkeying with var that = this.

Can't you use the fat arrow inside the step definitions in order to avoid this?

One more benefit (IMHO)- it will help people get off inheritance tree for worlds and orient them better towards the better powers of JS

Can you explain this a little more / give an example

@osher
Copy link
Author

osher commented Feb 15, 2017

In simple words: I belong to a school of programmers that have shunned the way of phrasing a context as a class. The more people get exposed to the dark side of OOP this school grows bigger - and the movement in the JS community that prevented arrow function from retaining context is an example of the strength of this movement.

Without getting into a religious debate - I'll just say I believe in live and let live, you want to use classes? great. Please don't force me to :-)

My general feeling is that it could be a small change, and I'll be happy to do it if you can provide me the general direction of how you think it would be done best ;-)

@mattbrunetti
Copy link

When we're defining steps, we're defining functions, not classes, so logically this has no place in it.

I think this is a situation for the FP paradigm rather than the OOP paradigm.

@mattbrunetti
Copy link

mattbrunetti commented Feb 15, 2017

@osher I think we can give @charlierudolph the benefit of the doubt that he doesn't want to force you to use classes. That said, he shouldn't be forced to integrate (breaking) API changes wherever he's using this package, unless it offers some tangible advantage.

@charlierudolph I think by "the better powers of JS" @osher means FP, Functional Programming.

IMHO this change would make step definitions a teeny bit simpler and more logical. It would basically just remove this line: const thisWorldNotThisStep = this

@mattbrunetti
Copy link

There's definitely no need to monkey .bind(this) to all your async functions, and as long as within the step definition you're using arrow functions, no need to use the const self = this pattern.

The problem (which isn't a big problem) is that inside a step definition, this refers to the world, not the step, which is counter-intuitive.

@charlierudolph
Copy link
Member

Okay. As not everyone is a functional programmer though and to prevent the introduction of a breaking change, what do you guys think of making an option which makes the world be passed as the first parameter?

defineSupportCode(({setWorldInjectionStrategy}) => {
  setWorldInjectionStrategy('this') // default
  setWorldInjectionStrategy('argument') // makes world be the first argument to steps and hooks 
})

@armandabric
Copy link

armandabric commented Mar 30, 2017

I totally agree with the idea to inject the world/context has a parameter of the step. Starting by this project, all the JS ecosystem is moving to fully embrace the new functionalities allowed by ES6 and that's a good things ❤️

If you look on other tools like Express or Koa, they set the current request context as the this AND as first parameter of the middleware (it is the equivalent of cucumber step). This solution allow traditional use of function and the use of ES6 arrow function.
Another advantage of the context in as the first parameter is that

For the solution proposed by @charlierudolph, I do not think this could works on a long run: this will split the community in two. All cucumber examples will not be working in all installations and the docs will be duplicated in two. Not fancy.

I'm not sure about the breaking change worries, the v2 is the perfect time to introduce this kind of change. Waiting will force to make/wait for another major.

@charlierudolph charlierudolph added the 🙏 help wanted Help wanted - not prioritized by core team label Apr 26, 2017
@nicojs
Copy link
Contributor

nicojs commented May 10, 2017

How about we change the Before and After to inject world:

defineSupportCode(function({After, Before, Given}) {
  let world;

  // Asynchronous Callback
  Before(function (w, scenarioResult, callback) {
    world = w;
    callback();
  });
  After(function (world, scenarioResult, callback) {
    callback();
  });

  Given('I access the world', () => {
    assert(world); // Yay!
  });
});

That way you can capture the world in a variable an a before step. It won't mess up each step definition.

Next, we keep the old way of working but generate a deprecated message. Or even don't do that, just leave the option there for people that want to use that approach and know what they are doing.

@osher
Copy link
Author

osher commented May 14, 2017

I'm afraid the argument about compatibility of snippets found online is a compelling argument.

Even if there will be --ui flags, or setWorldInjectionStrategy('argument') - it becomes a gotcha that is better communicated as a breaking change of a major version, served with all the online discussions and uproar befitting such changes, and helping to eliminate confusion.

So I vote for doing it in the next version of cuke, and ...makedoing until it releases.

A canary version or a hidden flag would be an awsome promotion I'd use and feedback early on

@jbpros
Copy link
Member

jbpros commented Jun 24, 2017

I'd love to expose an alternative FP interface.

What about the following? (using async/await to illustrate it's promise-friendly too)

import {initialize, Given, Before} from 'cucumber/fn'

// specify a function that returns the initial context:
initialize(async () => ({ a: 42 }))

Before({ timeout: 10 }, async (ctx) => {
  await doStuff(ctx.a)
})

Given(/^a step passes with {number}$/, async (ctx, number) => {
  const newA = await computeStuff(ctx.a, number)
  // tell cucumber about the new context:
  return Object.assign({}, ctx, { a: newA })
})

@charlierudolph charlierudolph added has pull request and removed 🙏 help wanted Help wanted - not prioritized by core team labels Jun 29, 2017
@aslakhellesoy aslakhellesoy mentioned this issue Jan 30, 2018
4 tasks
@jbpros
Copy link
Member

jbpros commented Dec 21, 2020

I've hacked together cucumber-fp that offers functional step definitions. I've got a few improvement ideas already. Feedback welcome!

I suggest we close this issue and let people experiment with that small lib. Maybe one day we can bring it into Cucumber.js.

@andyearnshaw
Copy link

andyearnshaw commented Jan 18, 2021

@jbpros I'd prefer not to have to install yet another dependency just so I can use arrow functions in my tests.

This should actually be really simple for everyone to implement themselves. The following code snippet works pretty well (functionally) for me, with a huge caveat that makes it unusable:

// Don't rely on `this` in step definitions. It's 2021 for crying out loud.
const definitionFunctionWrapper = (fn) =>
    function(...args) {
        return fn(...args.slice(0, -1), this);
    }

That caveat being every single step definition now logs the following error because of the additional parameter:

    Error: function uses multiple asynchronous interfaces: callback and promise
       to use the callback interface: do not return a promise
       to use the promise interface: remove the last argument to the function

If you try to add additional parameters to work around it, you get

function has 3 arguments, should have 1 (if synchronous or returning a promise) or 2 (if accepting a callback)

All cucumber-js needs is an option to disable function arguments checks and this problem will go away. Actually on second thought it looks like the test will time out too, as cucumber assumes that a callback is required based on the number of arguments in the step definition function. This could be worked around by giving precedence to a returned promise, though.

@jbpros
Copy link
Member

jbpros commented Jan 19, 2021

@andyearnshaw thanks for your input, I hear your concern about a dependency "just for arrow functions in stepdefs". This library is basically the solution I use personally to get stateless step defs, I just packaged it for anyone interested out there.

Consider it as a temporary solution around the ongoing debate for such a pure stepdef API in core (it's been going on for more than 4 years, believe it or not). As I said, this is an experiment that could land in core at some point and I would really appreciate feedback from people actually using it. If it gets enough traction, that'd make a better argument for integrating in cucumber.

Also, please note it is offering a couple of other (small) useful functional tools: tap() and enforced read-only contexts.

The arity check on stepdef functions is definitely a problem that I had to circumvent in this lib (in quite an ugly manner). An option to turn it off both on the CLI and programatically would be very useful for this (and potentially other use cases). I'd love to do that, but time is a scarce resource for me at the moment.

Feel free to get inspiration from cucumber-fp to fix the arity check in the meanwhile.

@andyearnshaw
Copy link

@jbpros that's great, and I really do appreciate the effort you have put in there. I was more in disagreement with the sentiment that this issue should be closed. I'll take a look at your library and see if it helps me work around that annoying check. 🙂

@jbpros
Copy link
Member

jbpros commented Jan 27, 2021

@andyearnshaw ha right, thanks for the clarification. I agree we shouldn't ditch this idea and keeping this issue open is probably a good way to keep things transparent, indeed.

@aslakhellesoy aslakhellesoy added ⚡ enhancement Request for new functionality and removed undecided labels Feb 2, 2021
@aslakhellesoy aslakhellesoy added this to Next in Cucumber Open Feb 2, 2021
@aslakhellesoy aslakhellesoy added the ✅ accepted The core team has agreed that it is a good idea to fix this label Feb 2, 2021
@aurelien-reeves aurelien-reeves removed this from Next in Cucumber Open Feb 16, 2021
@davidjgoss davidjgoss removed the ✅ accepted The core team has agreed that it is a good idea to fix this label Sep 20, 2021
@davidjgoss
Copy link
Contributor

Closing this now, because:

  • There are no current plans to address this from the core team
  • The fantastic https://github.com/jbpros/cucumber-fp package is available for those who want to work with this pattern - its evolution may lead it into this project someday as has been said before.

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

No branches or pull requests

9 participants