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

Arrow function support with world option #2004

Open
Pyrolistical opened this issue Apr 18, 2022 · 4 comments · May be fixed by #2402
Open

Arrow function support with world option #2004

Pyrolistical opened this issue Apr 18, 2022 · 4 comments · May be fixed by #2402
Assignees
Labels
⚡ enhancement Request for new functionality

Comments

@Pyrolistical
Copy link
Contributor

Pyrolistical commented Apr 18, 2022

🤔 What's the problem you're trying to solve?

It is such a common problem that users use arrow functions in step definitions, that this is a FAQ item https://github.com/cucumber/cucumber-js/blob/main/docs/faq.md#the-world-instance-isnt-available-in-my-hooks-or-step-definitions

This is likely due to the fact that popular testing libraries such as Jest use of arrow functions https://jestjs.io/docs/api#testname-fn-timeout

The problem is cucumber-js is violating Principle of least astonishment

✨ What's your proposed solution?

I propose an API breaking changes where world is passed in as the first parameter of arrow function.

BEFORE

Then(/^the response status is (.*)$/, function (status) {
  assert.equal(this.responseStatus, status)
});

BREAKING AFTER

Then(/^the response status is (.*)$/,  (world, status) => {
  assert.equal(world.responseStatus, status)
});

This could be introduced as an option with the default as the existing behaviour to allow users try the new behaviour without a breaking semver. Then once stable, release breaking semver and change the default behaviour. This also allows users who wish to upgrade to use option to restore previous default.

⛏ Have you considered any alternatives or workarounds?

No other obvious way to achieve this. The world could be added as the last parameter of the arrow function, but I think in the long run that is more confusing.

📚 Any additional context?

Related Github issues: https://github.com/cucumber/cucumber-js/issues?q=is%3Aissue+arrow+is%3Aclosed


This text was originally generated from a template, then edited by hand. You can modify the template here.

@mattwynne
Copy link
Member

This is consistent with the Typescript idiom, where you use this as the first parameter so you can define the type of your world.

@Pyrolistical Pyrolistical added 💔 breaking change This will require a major release ⚡ enhancement Request for new functionality labels Apr 18, 2022
@davidjgoss
Copy link
Contributor

I think the current API was right for its time but yeah if we were designing it now we'd probably do what you describe. We've previously balked at the breaking change but introducing as an option and giving a couple of major releases to switch over seems fair.

I could see an option like worldInjection with "this" (current default) or "argument" (future default) as values.

@Izhaki
Copy link
Contributor

Izhaki commented Jun 9, 2022

As first parameter

This is consistent with the Typescript idiom, where you use this as the first parameter so you can define the type of your world.

But Typescript is not Javascript; I wasn't even aware of this until I saw this in cucumber (#1948). Further, if this change is to be accepted, this will be typescript valid:

Then(/^the response status is (.*)$/, function (this, world, status) {
  assert.equal(this.responseStatus, status)
});

which is super-confusing.

In addition, not everyone is using world, so adding it as first parameter may make it unused for some.

So if it is an additional parameter, I'd vote for it to be last.

How others solve it?

World and this are more of an OOP paradigm. With React Class Components you'd have:

this.context

Whereas with Functional Components the API is:

const context = useContext(MyContext);

So I believe this is doable:

import { Then, getWorld } from '@cucumber/cucumber';

Then(/^the response status is (.*)$/,  (status) => {
  const world = getWorld();
  assert.equal(world.responseStatus, status)
});

This leaves world as opt-in in arrow functions + I believe will solve the typescript issue (#1667).

OOP vs functional (again)

Understandably, the current API involves a World class and rely on this (OOP paradigm). Arrow functions are largely associated with more functional style. Similar to React, the "OOP" here is largely syntactic (no inheritance).

So the question we could ask is "had we designed it ground-up today what would be the solution?".

In a typescript project I was reluctant to adopt the this solution, so I decided to try and simply have a global variable called context and just interface with it in step definitions. This is not a full blown World, but you could easily provide createContext that will decorate the object with attach at al, like so:

export const context = createContext<Context>({
  uid: string,
})

Now I haven't thought this through and through, and I'm not really pitching this as a possible solution.

But the takeaway is that instead of trying to bridge two conflicting paradigms, maybe the way is just to accept these are two different paradigms, and support both in the right way (so this for function, world or getWorld for arrow functions).


PS
Shouldn't this issue heading be prefixed with RFC: ?

@davidjgoss
Copy link
Contributor

davidjgoss commented Jul 19, 2022

@Izhaki thanks for those thoughts, and sorry for the late reply.

But the takeaway is that instead of trying to bridge two conflicting paradigms, maybe the way is just to accept these are two different paradigms, and support both in the right way

Very fair!

Parameter placement

I agree the analogy to TypeScript's this is a little tenuous. However, I think I'd still choose to put it first rather than last. Cucumber checks the number of arguments in the function to a) ensure the arguments match the step arguments and b) to see if it should pass a callback function for old-style async code. This would make it somewhere between difficult and impossible to have the world as an optional last param, and as @Pyrolistical noted confusing to the user. To be fair, even with it as the first param, we'd be adding some complexity by having the backwards-compatible option.

Hooks-style solution

I think this could be good. The React comparison makes sense. I'd even be tempted to name it useWorld()! Thinking about TypeScript and custom worlds, users could easily wrap it in a type-specific flavour to avoid having to explicitly type it in every function which is what you have to do now.

I decided to try and simply have a global variable called context and just interface with it in step definitions

The world is one instance per scenario, so not quite "global". However we are only executing one scenario at a time per process (for the parallel runtime we spawn child processes) so it should be fine to implement hooks-style without too much complexity.

I'll try and do a POC for this soon.

@davidjgoss davidjgoss removed the 💔 breaking change This will require a major release label Sep 4, 2022
@davidjgoss davidjgoss self-assigned this May 6, 2024
@davidjgoss davidjgoss linked a pull request May 6, 2024 that will close this issue
7 tasks
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

Successfully merging a pull request may close this issue.

4 participants