Remove Guaranteed from CellSuccessData

[pvenable]

Alternative to #8186. Addresses #8179 (comment).

#7704 was a fairly significant Cell behavior change in Redwood 5. Success can be rendered with partial data, but CellSuccessData was not updated to reflect the change. Other variables such as whether isEmpty or Empty are defined in a Cell can also make it possible to render Success with partial data even in cases that would otherwise render Empty instead.

This can lead to runtime errors that are better made impossible to ship via typing that accurately reflects the possibility of missing data even in Success.

Unfortunately I don't see any obvious way to conditionally "guarantee" the presence of data at the type level based on the various factors that can impact it (custom isEmpty implementations, missing Empty definitions, or the number of fields in your GraphQL query result), so it seems the only safe thing to do is to stop representing that all data properties are guaranteed to exist when rendering Success.

Side note: At least under strict typing, this change makes "automatic" Empty somewhat less useful since it's now necessary to do presence checks in Success anyway. I suspect Empty will generally remain useful mainly for the "empty array" case and less for the "null result" case.

[pvenable]

These comments are no longer accurate because we render Success if any property is present/non-empty. Other properties can still be missing/empty.

A custom isEmpty implementation or a missing Empty definition can also result in Success.

[pvenable]

This would only be true when all of the following are true:

• Empty is defined
• isEmpty is not customized, or its custom implementation retains the presence check (which is admittedly likely 🙂)
• post is the only field in your graphql query, or all fields are present/non-empty

[Tobbe]

@pvenable Thanks for working on this. I haven't done a proper review yet, but reading through the PR description and your comments I like where this is going 👍

One thought that came to mind is if we could still keep Guaranteed or something like it for simple cells without isEmpty and with just one root field by having two different types - one with Guaranteed and one without - and then use our codegen to choose between the two. Perhaps by generating a Success prop inside the Cell type file (e.g. .redwood/types/mirror/web/src/components/MyCell/index.d.ts) and use that instead of CellSuccessProps.
Again, this was just a thought I had. Might not work.

[pvenable]

@Tobbe Thanks for the comment.

One thought that came to mind is if we could still keep Guaranteed or something like it for simple cells without isEmpty and with just one root field by having two different types - one with Guaranteed and one without - and then use our codegen to choose between the two. Perhaps by generating a Success prop inside the Cell type

Yes! -- I'm hoping for something like this too in the long run. There are many happy cases where the data could be guaranteed, and the best experience would be to make it automatic, such that CellSuccessData (or equivalent) is always in sync with any changes to your cell that could affect it.

I tried to figure out a way to do something like this solely via Conditional Types a little while ago, but to my surprise I couldn't find a safe/reliable way to count the properties of a type in the type system. 😬 Combined with the isEmpty/Empty issues, my conclusion was that it would indeed require codegen to accomplish this goal -- and I haven't had a chance to look into that further.

I guess an interim option could be to have e.g. both CellSuccessData and GuaranteedCellSuccessData, and just manually opt in to the latter when you know the criteria are met. At least this would give a safe default.

I do consider the current CellSuccessData a bit of a land mine, so I look forward to hearing whether a fix like the one in this PR can stand alone (or perhaps with GuaranteedCellSuccessData). It seems like any codegen-based solutions we're discussing could come later. That said, I could probably switch all Cells in our codebase over to a custom non-guaranteed CellSuccessData in the meantime if the codegen solution is required to resolve this inside of Redwood.

[Tobbe]

I'm more than happy to do this in steps, with a codegen solution later. I'll bring this PR up with the rest of the team to get their input

[Tobbe]

@pvenable So we discussed this today. We agree that the current behavior is wrong, and just making everything optional (i.e. getting rid of Guaranteed) would make it technically correct. Unfortunately that'd be a breaking change for people who have just basic cells with a single root query. They'd now need to add an undefined check to their cells. And we'd have to update our tutorial to do that too. So while we could merge this as it is, we couldn't release it until v7. And seeing how v6 just went out v7 is probably not going to happen for a couple of months.

With all that said, we'd prefer not to merge this PR as it stands right now.
If you could try some codegen trickery though, to at least detect default generated cells and give them the correct Guaranteed version of the props that'd be very welcome.
If we had a sufficient clever codegen solution in place we'd feel much better about merging, and also releasing in an upcoming minor release.

Please let me know what you think about the reasoning above. And definitely tell me if it sounds like I might have misunderstood something about your current solution.

[pvenable]

Thanks for the response. Here are some of my thoughts, and I apologize in advance for my verbosity. 🙂

We agree that the current behavior is wrong, and just making everything optional (i.e. getting rid of Guaranteed) would make it technically correct.

Strictly speaking there's no (runtime) behavior change in this PR. It's only correcting a type that misrepresents as guaranteed-to-be-present that which is not. There is not currently enough information at build time to guarantee this data. There's not necessarily anything wrong with the current behavior of Cells; it's just that we can't be assured that the data will be present due to factors that can't be (or aren't yet?) statically assessed.

Note that this can also make it difficult to properly test Success or handle missing data as the types don't allow for it -- even though it's possible. In our project, for example, linting will warn about and auto-fix unnecessary conditions (such as optional chaining). (As I will discuss below, this may only be an issue under TS strict mode.)

Unfortunately that'd be a breaking change for people who have just basic cells with a single root query. They'd now need to add an undefined check to their cells.

They may already need to the moment they add another query field or remove Empty. But nothing will warn them about it -- it may just blow up at runtime, possibly in production.

For what it's worth, I think this is only a "breaking" change to users of TypeScript strict mode, who (like me) would almost certainly want this type to accurately reflect the potential optionality of these data fields absent a way to conditionally guarantee it according to actual Cell behavior. I see it rather as a "fixing" change. 🙂

After a cursory experiment, I don't think this change is even observable to non-strict TypeScript, but I admit that since I always use strict mode I'm not familiar with any potential "gotchas".

we'd have to update our tutorial to do that too

The tutorial states this in Chapter 2 (Cells): "The minimum you need for a cell are the QUERY and Success exports. If you don't export an Empty component, empty results will be sent to your Success component. If you don't provide a Failure component, you'll get error output sent to the console."

So it's well-known that empty results can be sent to Success -- but the types don't represent that. Defining a cell without the Empty component (assuming a non-array result) will generally result in a runtime error that should have been prevented by TypeScript (at least in strict mode).

If you could try some codegen trickery though, to at least detect default generated cells and give them the correct Guaranteed version of the props that'd be very welcome.

Unfortunately, this is likely more than I expect to be able to commit to in the near future.

With all that said, we'd prefer not to merge this PR as it stands right now.

Please let me know what you think about the reasoning above. And definitely tell me if it sounds like I might have misunderstood something about your current solution.

I understand the tension around the "breaking" change, and particularly the awkwardness of accepting at the type-level that we can't (currently) guarantee that Success data will be present. This clearly feels bad in the context of how Empty is supposed to work. But in practice, at least under TypeScript strict mode, the inclusion of Guaranteed<T> in the success typings just leads to otherwise-preventable runtime errors outside of the happy case.

Ultimately it's up to Redwood whether and when this or any solution for this is integrated in terms of release cycles. Around 20% of our project's cells do not meet the single-root-query happy path criteria, and it's too easy to make the mistakes this PR prevents, so if now is not the right time for a Redwood fix, I think I can make this change locally for our project as an interim workaround.

[Tobbe]

Thanks for your thorough response @pvenable. I really appreciate you taking the time to write that all down.

I agree with pretty much everything you said. And it sounds really promising if this would only be breaking for strict mode users. If that's the case I think we can merge this sooner rather than later.

And, just to clarify, we generally consider it a breaking change if code goes from non-red to red, even if it's "just" types and the runtime behavior stays the same.

Unfortunately, this is likely more than I expect to be able to commit to in the near future.

Totally understand. And grateful for the time you've already spent on this. We'll leave that task open for some other contributor, or a core-team member, to tackle another time :)

[pvenable]

Thanks, @Tobbe.

And, just to clarify, we generally consider it a breaking change if code goes from non-red to red, even if it's "just" types and the runtime behavior stays the same.

Personally, I see this as similar to a "breaking" 6.0.3 (patch) change accepted via the rationale given here: #8918 (comment). With that said, I do recognize that this is a more complicated case since it's technically only broken "sometimes", and Cells have an established behavior and history one wouldn't want to hastily destabilize, so I can definitely appreciate the reasoning here.

I am planning to implement roughly this PR's solution directly into our current project until Redwood integrates a fix, so the urgency from my perspective is somewhat relieved. I'm also not married to this particular implementation if there are any misgivings about it, or if a "better" solution can be devised in the meantime.

I appreciate the consideration and discussion. 👍

[pvenable]

I couldn't stop thinking about this, so I've proposed an alternative in #9037. 🙂

[jtoar]

Closing since #9037 has been merged instead.