feat(di): support state objects requiring other state objects

[tomtomau]

G'day 👋

We've just started using this package and it's pretty sweet!

One of the things we've tried to do is to utilise functionality more like dependency injection. What we often want to do is create a "service" which can contain some state or some methods to affect the system under test and modify the state.

As we've added more test cases in, we've wanted to be able to then this "service" have another "service" injected into it etc.

The @binding() functionality functions a lot like lightweight dependency injection for our use cases, but falls short that anything that has @binding() can't be "injected" to something else with binding()

This was primarily enforced before with CustomContextType which has been loosened up to allow this use case.

There's additional specs to cover some different ways to use it, including explicitly catching issues that arise from circular dependencies (the constructor prototype is undefined in these cases)

Let me know what you think, happy to add any more tests as necessary 😀

[cborgas]

Nice

[timjroberts]

Looks good to me, and a great completion to the DI side of bindings.
I'll defer to @Fryuni for a full review though.

[Fryuni]

There is a JS oddity there.
If you want to tackle that edge case in this PR it would be great, but that is not required. I can open an issue for that and fix it when I have time (or someone else can beat me to that).

It was quite smart to defer the problem to Node given it already handles it, unluckly it just doesn't perfectly here.

Besides that quirk, great work.

[Fryuni]

This detection relies on how Node's resolves a circular import.
Declaring two classes on the same file may cause a false positive and calling the binding manually would cause a false negative.

A side note, on the horrors of JavaScript.

Circular reference and circular dependencies are both valid in JS, only circular construction is not allowed. In fact, it is not really unallowed... it is just impossible to write it with JS syntax, but you can do it through Reflect and Object.create.

This is entirely valid and works as "intended":

// -- one.ts --
import {binding} from './binding-decorator';

export class OneA {}

// Two depends on OneA, which was already exported
import {Two} from './two';

@binding([Two])
export class OneB {}

// -- two.ts --
import {binding} from './binding-decorator';
import {OneA} from './one';

@binding([OneA])
export class Two {}

Reporting circular imports as errors is something that I'm totally ok with, they should be avoided anyway.

But this doesn't actually detect circular dependencies if the decorator is called manually. This will cause an infinite recursion and break the running test with RangeError: Maximum call stack size exceeded.

import {binding} from 'cucumber-tsflow';

export class StateOne {
    constructor(public stateTwo: StateTwo) { }
}

@binding([StateOne])
export class StateTwo {
    public value: string = "initial value";
    constructor(public stateOne: StateOne) { }
}

exports.StateOne = binding([StateTwo])(StateOne);

Circular dependencies can be detected and reported precisely by walking the chain of required context looking for loops.

[tomtomau]

@Fryuni I've re-requested your review on this one, I've added in a spec covering the example you provided and it surprisingly doesn't throw RangeError: Maximum call stack size exceeded.

It does however probably hint to the "wrong" error, it says Undefined context type at index 0 for StepsTwo, do you possibly have a circular dependency? when I think it would ideally be mentioning something about StateOne and StateTwo being circularly dependent.

[Fryuni]

Yeah, Node makes one of the imports become undefined (due to the ordering of its resolution).
You've hit the case of the false positive, it is reporting that a class has a circular dependency when it doesn't. Although... this is kinda open to interpretation, it is not part of the cycle, but there is a cycle in its dependency tree. So your new scenario of reporting it without the exact name is fine.

I wrote a scenario for the other, IMO more serious, scenario when reviewing:

    Scenario: In-file circular dependencies are explicitly communicated to the developer
        Given a file named "features/a.feature" with:
            """feature
            Feature: some feature
              Scenario: scenario a
                Given the state is "initial value"
                When I set the state to "step value"
                Then the state is "step value"
            """
        And a file named "support/circular.ts" with:
            """ts
            import {binding} from 'cucumber-tsflow';

            export class StateOne {
                constructor(public stateTwo: StateTwo) { }
            }

            @binding([StateOne])
            export class StateTwo {
                public value: string = "initial value";
                constructor(public stateOne: StateOne) { }
            }

            exports.StateOne = binding([StateTwo])(StateOne);
            """
        And a file named "step_definitions/one.ts" with:
            """ts
            import {StateTwo} from '../support/circular';
            import * as assert from 'node:assert';
            import {binding, when, then} from 'cucumber-tsflow';

            @binding([StateTwo])
            class Steps {
                public constructor(private readonly stateTwo: StateTwo) {
                }

                @when("I set the state to {string}")
                public setState(newValue: string) {
                    this.stateTwo.value = newValue;
                }

                @then("the state is {string}")
                public checkValue(value: string) {
                    console.log(`The state is '${this.stateTwo.value}'`);
                    assert.equal(this.stateTwo.value, value, "State value does not match");
                }
            }

            export = Steps;
            """
        When I run cucumber-js
        Then it fails
        # And the output does not contain ""
        And the error output contains text:
            """
            Undefined context type at index 0 for StateOne, do you possibly have a circular dependency?
            """

Uncomment the line at the end to see the error message that happens at runtime for the test of the consumer of this library.

[tomtomau]

Legend, thanks, I'll poke around and see if I can implement something for this.

Would it logical to merge this PR, adding in your spec you just posted of the in-file circular dependency specifying how it currently works, even if it's not the most ideal way

Proposed spec showing less than ideal behaviour, note the `Then`

Scenario: In-file circular dependencies are thrown as maximum call stack exceeded errors
    Given a file named "features/a.feature" with:
        """feature
        Feature: some feature
          Scenario: scenario a
            Given the state is "initial value"
            When I set the state to "step value"
            Then the state is "step value"
        """
    And a file named "support/circular.ts" with:
        """ts
        import {binding} from 'cucumber-tsflow';

        export class StateOne {
            constructor(public stateTwo: StateTwo) { }
        }

        @binding([StateOne])
        export class StateTwo {
            public value: string = "initial value";
            constructor(public stateOne: StateOne) { }
        }

        exports.StateOne = binding([StateTwo])(StateOne);
        """
    And a file named "step_definitions/one.ts" with:
        """ts
        import {StateTwo} from '../support/circular';
        import * as assert from 'node:assert';
        import {binding, when, then} from 'cucumber-tsflow';

        @binding([StateTwo])
        class Steps {
            public constructor(private readonly stateTwo: StateTwo) {
            }

            @when("I set the state to {string}")
            public setState(newValue: string) {
                this.stateTwo.value = newValue;
            }

            @then("the state is {string}")
            public checkValue(value: string) {
                console.log(`The state is '${this.stateTwo.value}'`);
                assert.equal(this.stateTwo.value, value, "State value does not match");
            }
        }

        export = Steps;
        """
    When I run cucumber-js
    Then it fails
    And the output contains "RangeError: Maximum call stack size exceeded"

We've forked & published an internal package so we're not blocked by any of this, so it's just up to you folks as to whether you'd want to merge this in as is with this additional spec, and then we can create an issue to fix it for the ideal behaviour?

[Fryuni]

I'll poke around and see if I can implement something for this.

For such a use case keeping an array of traversed constructors is enough. It is O(n²), but I doubt the dependencies will get deep enough for it to matter.
It also has the advantage that you can then print the entire cycle once it is detected so the error message can be quite clean.

We've forked & published an internal package so we're not blocked by any of this, so it's just up to you folks as to whether you'd want to merge this in as is with this additional spec, and then we can create an issue to fix it for the ideal behaviour?

Since Node has a sensible stack limit and isolates such errors from other handlers in the event loop I think it is fine to merge this as is with the addition of your proposed spec. If it grew unbounded or the limit was beyond the available memory on an average machine than I'd like it fixed before merging.

Pls add the new spec and then I'll merge.

[tomtomau]

Pushed @Fryuni!

[Fryuni]

Thanks!

I'll cut a release with the feature and once we improve the cycle detection and reporting we can cut a patch :)