Shallow clone context

[nlf]

This closes #982

[kanongil]

Why change this? The feature is esoteric, and not used inside the hapijs org (outside of testing the feature in lab itself).

I say, if we want to do a breaking change, just remove the feature entirely.

[nlf]

why remove it entirely? there's some value in discouraging mutation of the context in tests to prevent one test relying on another test having passed.

this change is intended to allow you to do things like attach a server or a database client to the context in your before/after functions, which is a common pattern and does not work today.

[devinivy]

I'm personally for keeping it with this fix, though I don't have especially strong feelings. Aside from encouraging good hygiene, it's also sort of convenient over initializing variables then assigning them in before() since it's such a common practice.

[kanongil]

Common pattern where? It is not used in any hapijs org repos.

When you remove the cross test deep clone part of the feature, what remains is a shorthand to defining variables in the before() scope.

describe('whatever', () => {

    before(async ({ context }) => {

        context.server = await startServer();
    });

    it('does something with a server', ({ context: { server } }) => {

        expect(server.started).to.be.true();
    });
});

vs.

describe('whatever', () => {

    let server;

    before(async () => {

        server = await startServer();
    });

    it('does something with a server', () => {

        expect(server.started).to.be.true();
    });
});

While it can save a line of code, it adds an extra argument to tests, and loses any possibility of type inference for eg. autocompletion.

Also, this is not a bugfix, but a breaking change.

[devinivy]

For what it's worth, it is recognized that this is a breaking change— it would go into v24 with #992. I think that slight aesthetic difference between your two examples is exactly what people intend to get out of the context feature (outside the hapi org). When I said "common" I meant that it's common to share some resources across tests, as shown in your second example.

[nlf]

as the commit messages state, this is known to be a breaking change and not a bug fix. i added the breaking changes label to it to make that even more clear.

as @devinivy mentions, your example clearly illustrates how people expect it to work, which is not how it works today. yes, it is a convenience change and historically would've been rejected since it is not necessary for how it is consumed by the hapijs org today, but we live in a brave new world where useful changes can be accepted regardless of whether or not the hapijs core uses them.

also of note is that there were no tests to ensure that the context was a deep clone, which also implies that the deep cloning is not a feature that hapi relied on in core. so if hapi doesn't rely on the existing behavior, why would we not make it more intuitive for end users?

i'm not sure i follow your concern about type inference, a shallow clone and a deep clone would be the same inferred type would they not? the existing behavior already allowed the context to be a different type across experiments and that hasn't changed.

[devinivy]

Cool, let's figure this out. It seems like we have three options:

• close this PR and release v24 without any changes to this feature.
• wait to release v24 until we come to consensus on this question and possibly do some additional development.
• release this work in v24 and continue the conversation.

Given that this work is already complete and seems to be strictly an improvement to an already-existing feature (with an open issue #982), I propose we release it in v24 but create an issue to discuss and come to consensus whether the context feature should be removed altogether. How does this sound to everyone? Gotta love meta-consensus :)

[Nargonath]

I'm in favor of keeping the context feature for now. Personally I don't use it much but there seems to be people who do. I think it is a good idea to open an issue afterwards and discuss it to see if there is an interest in deleting the feature altogether if we really want to go that route.

[kanongil]

FYI, I'm fine with changing it. It just seems to make the feature less valuable.

I will need to change a couple of tests that rely on the cloning behaviour, where I will replace it with a solution without the context object, and thus wondered whether it even makes sense to keep around.

[kanongil]

i'm not sure i follow your concern about type inference, a shallow clone and a deep clone would be the same inferred type would they not? the existing behavior already allowed the context to be a different type across experiments and that hasn't changed.

The type inference argument does not apply to this change, but in general to attaching stuff to the context object, vs. a scoped variable.

[devinivy]

I will need to change a couple of tests that rely on the cloning behaviour

@kanongil I think this would be useful to see, since currently nobody has shown a concrete use-case for the deep cloning behavior, but we have seen its drawbacks (e.g. you can't place a hapi server on context). Could you give a brief example?

[kanongil]

Sure, the code is here.

Of course, it won't actually break with this change, since I only modify the object that one place – and it can be fixed simply by changing the before() to a beforeEach().