Polymorphism data management is a mess

[benjie]

I've overhauled the external APIs around polymorphism so that the public interface shouldn't need to change (including hiding things like $$data, $$polymorphicType, etc internally), but internally it's a mess. Currently we take the object you feed us and we mutate it to attach the $$polymorphicType information:

https://github.com/benjie/postgraphile-private/blob/58b875c1e8a64d0165a635be6832301e0e091bd0/grafast/grafast/src/polymorphic.ts#L43-L51

What I think we should do instead is:

1. polymorphicWrap returns a {$$polymorphicType: type, $$data: data} object
2. when we receive this data in execute, we split it and store it into two separate stores, one for the data/error/etc as usual, plus a new store that just stores the type (or null) - we could do this by appending _type to the identifier if we used strings, but since we use numbers adding 2 billion to the step ID should be sufficient.
3. Most of the execution only cares about the actual data, so that can stay as is (which mirrors the interface now exposed), but the internal parts that care about the type will need to read from this new secondary store to determine which branch to follow.

I think this can be done after the 5.0.0 release because I think this is all internal.

[jemgillam]

This could be addressed in v2 perhaps instead of before v1 is released.

[tsnobip]

while testing postgraphile v5, it was for me by far the hardest part to understand.

I think it'd be good to add a helper step for what I think is a common use case, when you build a polymorphic object yourself in a polymorphic-uncapable step (which most steps are). You can't use directly polymorphicWrap inside the step callback in such cases because the step doesn't implement planForType.

So we could provide a polymorphic step that would take the result of the step, call polymorphicWrap on it with the type being taken from the object __typename field by default and make it implement a dummy planForType given there won't be any logic in it.

What do you think?

[benjie]

I'm not convinced that we actually need plans to implement .planForType(...). We had a similar issue with .listItem(...) for planning lists - at first this was required, but I realised that in a lot of cases the step was just returning the incoming item, so I made it optional. I think we should do the same for planForType - if it's present then we enable branching to different steps based on the type, but if it's not present then we just trust that the step will have called polymorphicWrap (in the same way we trust that steps used in list positions will return an array) and if that's not the case we can just throw a run-time error.

[tsnobip]

Yes, that would be even better, planForType is not always necessary indeed.

[benjie]

I'm not really happy with polymorphism in Grafast in general; I need to build out more examples in more technologies and then figure out how it should work, and then refactor to that. Currently it does work, but it's, as you say, the hardest part to understand. (This is probably related to the fact it was the hardest part to actually implement.)

[benjie]

Just reviewing issues and I noticed this one; @tsnobip we now have https://grafast.org/grafast/step-library/standard-steps/polymorphicBranch

[tsnobip]

Just reviewing issues and I noticed this one; @tsnobip we now have https://grafast.org/grafast/step-library/standard-steps/polymorphicBranch

wow, that's exactly what I was looking for, thanks a lot @benjie!