(WIP, DON'T MERGE) Fix typings in scenes/context.ts

[Evertt]

Fixes #1827

[MKRhere]

lgtm, thanks!

[MKRhere]

This will need a fix in Wizard Context; but before that, does this fix even work as expected for you?

[Evertt]

This will need a fix in Wizard Context; but before that, does this fix even work as expected for you?

I initially wanted to type it as Exclude<D["state"], undefined>, but TypeScript wouldn't let me. But for some reason, when I type it as D["state"] it's already non-optional in my IDE. Which, to be honest, doesn't really make sense it me. But whether it comes out as optional or non-optional, both of those are already better than simply object in my opinion.

Edit

I'm happy to add this to the Wizard as well. I didn't do it thus yet, because I haven't been using the Wizard so I didn't run into the same problem.

[Evertt]

@MKRhere Would you want me to apply this fix in the Wizard scene as well? Because I just tried it and I noticed that the wizard scene is set up quite differently from the base scene. And so I would be inclined to refactor it such that it works much more similar to how the base scene works. But I don't know if you guys made it different on purpose.

[MKRhere]

We cannot change the behaviour within a major version, and even for v5 there must be justification for the change. Right now, we just want Wizard scene to work along with this change, so there are no type errors.

[Evertt]

We cannot change the behaviour within a major version, and even for v5 there must be justification for the change.

Okay, well I can think of a justification? Why would you allow to explicitly define a D extends SceneSessionData in scenes/context.ts:

export default class SceneContextScene<C extends SessionContext<SceneSession<D>>, D extends SceneSessionData = SceneSessionData> {}

But not do the same for the Wizard?

export default class WizardContextWizard<C extends SessionContext<WizardSession> & {
    scene: SceneContextScene<C, WizardSessionData>;
}> {}

I can't see any good reason for this disparity. So I'd propose that in v5, WizardContextWizard will basically exactly like SceneContextScene:

export default class WizardContextWizard<
    C extends WizardContext<D>,
    D extends WizardSessionData = WizardSessionData,
> {}

And then also the property readonly state: object could be turned into a getter and setter just like in SceneContextScene.

And from what I understand, v5 is just around the corner, so we can wait with implementing this PR until v5 for all I care. :-)

[MKRhere]

That would be a non-breaking change, because existing code doesn't break if D is not passed. Please make that change and we can land it in v4.

[Evertt]

@MKRhere I just pushed a new commit. Now I've really committed to getting it right. So it gets correctly compiled by tsc and also all the tests run by npm run test now run successfully.

I am aware that I changed the required generic types of some types. This was the only way to make it work and be type-correct.

If that means it can't be accepted for v4, shall I then make a new PR for v5?

edit

Wait, I still made a mistake, which was not caught by tsc or the test run, but I caught it by using it in my own code. I'll make a second edit once I've fixed this mistake.

edit 2

@MKRhere Okay now I fixed all of it. You can try it out for yourself. It's much more type accurate now and it even shows you a warning text when you're giving the wrong type!

edit 3

Nope, still not all of it is fixed. It's clear that you guys did not anticipate that each scene may want its own well-defined state.
And so now the Stage class is confused, and it's taking me too much time to fix it. 😅

So, yeah, don't merge this PR, consider it a Work In Progress

Let me know if you see any value in this though? In the end, I'd like to allow the user to define a unique state type for each scene, and for the Stage class to not be bothered by that.

And even, if one does something like:

const { enter, leave } = stage

enter<WelcomeSceneState>({ /* whatever fill in the data that the welcome scene expects... */ })

[oitan]

Is there a plan to merge this? Current types related to session, scene are broken by my experience. Or is this not related to it?

[Evertt]

I'm sorry, I've been struggling with a depressive episode recently and I haven't been able to get myself to finish this or anything else. If someone else would be willing to take this over then I'd appreciate that.

[oitan]

I'm sorry, I've been struggling with a depressive episode recently and I haven't been able to get myself to finish this or anything else. If someone else would be willing to take this over then I'd appreciate that.

I'm sorry to hear that. I hope you will get better soon! If this will not be a burden to you, can you please let me know what's left and push all the progress you have? I would appreciate it.

[MKRhere]

@Evertt Please do not stress yourself, take your time to recover. I'll look into this once I have time.

Current types related to session, scene are broken by my experience

@oitan This PR is only related to ctx.scene.state not being strongly typed. Generally session types are sound. If you have a different problem, please make an issue, and we'll look into that.

[SwapnilSoni1999]

Eagerly waiting for this to be merged! :) thanks sir

[Evertt]

Eagerly waiting for this to be merged! :) thanks sir

@SwapnilSoni1999 Hey, well the good news is that my latest depressive episode seems have finally come to an end. The bad news is that my life is now much busier so I still can't promise that I will finish this.

But if anyone else is willing to pick up the slack then I very much welcome that. Why not you? 😉