Omit fields extra typescript option

[SamProtas]

This PR adds omitFields to ExtraTypeScriptOptions. The general ideas was taken from this similar library for generating mobile types from Haskell: https://github.com/MercuryTechnologies/moat

The motivating example is better support for breaking changes to TypeScript types.

When you want to deprecate a record field, to replace it with another field of an incompatible type, typically there's a period of time in which the backend will send both fields simultaneously. This allows existing web clients that have not received the new frontend bundle to continue working, while simultaneously supporting a new frontend bundle that makes use of the new field. Without omitFields, the generated TypeScript will include the deprecated field that only exists for older clients. This forces new client code to deal with both fields or humans to edit the generated TypeScript.

Here's a concrete example, imagine you have an existing record:

data User { name :: String, age :: Int }
$(deriveJSONAndTypeScript defaultOptions ''User)

Let's say you realize what you really want on the client "birthday", not "age". You could do this:

data User { name :: String, age :: Int, birthday :: Day }
$(deriveJSONAndTypeScript' defaultOptions ''User defaultExtraTypeScriptOptions {omitFields = ["age"]})

You can safely deploy this change immediately, as existing web clients will still be sent the age key they expect. You can also cleanly develop a change using birthday, since your types no longer contain age. It'll be trivial to ensure the entire client has migrated, so removing age later (after enough time for existing clients to upgrade) will be very safe.

I've pulled this change into a large production codebase to confirm it works in practice.

There's a potential followup PR to add omitCases. The general idea is the same, but it allows for excluding sum type cases, rather than product type fields.

[SamProtas]

Hey @thomasjm thanks for the review!

It sounds like there are questions about API design, feature completeness and the underlying problem itself. I wrote some more detailed responses to your thoughts below.

1. I agree the stringly-typed nature of the API is less than ideal, but I haven't been able to come up with something better. I think if Haskell had "first class record fields" that would be a natural improvement. Alternatively, libraries such as persistent take the approach of using TemplateHaskell to generate a GADT representation of a record's field. We could do something like this, but I believe that would require a second splice above the use of omitFields to generate this type, and it also feels a bit heavy weight (maybe this is a personal bias though). Unfortunately the underlying type [Name] illustrates the lack of type safety available for this. Do you think better documentation or perhaps a better name (not omitFields) could improve this situation? Do you see a path forward for a different API?

2. Responding to this point is somewhat dependent on the path forward for (1). In the event the same basic API structure is kept, I'm open to adding the omitCases-like functionality to this PR so the API surface is complete.

3. I think the @deprecated suggestion is a great idea, and definitely a useful workflow for deprecating a widely used field where a clean cutover in one PR would be too burdensome. That said, I do want to push back a bit on this sentiment: It's a little weird to me to have the TypeScript definitions deliberately not match the results returned from the backend. I'd argue this is an inescapable situation that occurs during deployment of all client-server single page apps. There will always be a difference between the moment a particular client's web bundle is updated and when the associated backend cuts over. Usually there's a non-trivial amount of time where clients are on both the older and newer bundle, and when backend servers send both old and new API responses (rolling deploys). This workflow acknowledges that reality and allows the author explicitly define an "order of deploys". For example, our app enforces a maximum web bundle age. Using this workflow we can remove a field from the frontend types (but still send it in the JSON), wait a predetermined amount of time, then remove it from the JSON with no risk of breaking existing web sessions. During this waiting period, other engineers can regenerate types for unrelated changes without accidentally re-introducing a field we're removing. If you exclusively rely on @deprecated to remove fields, there will be a deploy removing the field from the backend and the frontend "simultaneously". The removed frontend type will be deprecated, but in a large codebase (with potentially other deprecated fields) it can be hard to ensure it's not accessed anywhere, especially in recently merged PRs authored by others.

I suppose a good place to start is confirming if you're willing to accept a PR addressing the underlying problem as I've defined it? I think your @deprecated suggestion is a good feature idea, but I consider it more of a complimentary feature, as it wouldn't fully address our issue. I think you raise good points about the API design, but unfortunately I don't have suggestions for a better one. Assuming we find common ground on the above, I can complete the feature as discussed in (2). Any suggestions for a path forward?

[thomasjm]

Hey, I still need to think about most of this a bit more but wanted to clarify on this part --

If you exclusively rely on @deprecated to remove fields, there will be a deploy removing the field from the backend and the frontend "simultaneously".

Not at all, a deprecated field can remain on either the frontend or backend as long as necessary right? To my mind, marking a field deprecated on the frontend is equivalent to removing it, since you can tell TS to treat usages as errors. Better even, since the error will come with a helpful message.

Is the issue that you can't treat all frontend deprecations as errors, because some are expected to remain in the codebase? If so, I wonder if there's a way to configure certain deprecations to be errors and others warnings...

[SamProtas]

Is the issue that you can't treat all frontend deprecations as errors, because some are expected to remain in the codebase?

Yes I think that's the main problem I see with your suggestion for our workflow. Given the size of our engineering team and the frequency of commits, it's very hard to make huge changes all at once. Changes like these get perennially stuck as new merge conflicts arise as fast as old ones are fixed. We use deprecation today to allow for a slow migration away from deprecated things. It prevents most new uses, while allowing small targeted PRs to remove old uses, until the total use count approaches zero. Then we'd ideally drop the field from the type (what this PR enables) which allows us to guarantee it's not being used or added back.

I can understand wanting to keep the API surface of the library small and focused, but I'm hopeful this change is both sufficiently small and the use case sufficiently general that you'll accept it upstream. I really do think this is something other users of this library could benefit from.

[thomasjm]

Btw this is still on my radar and I feel properly guilty for neglecting it--I'm still a bit busy at the moment but I'll get back to it as soon as possible.

[SamProtas]

Hey @thomasjm I appreciate the update and no need to feel guilty about the delay. That said we do still have this problem! If there's anything I can do to help this along (additional justification, adding omitCases, API tweaks) I'll be happy to address that.

[thomasjm]

Hey, so I finally got a chance to play around with the Haddocks idea I mentioned and I think I've come up with a good solution for this.

First I added the machinery for reading the Haddocks from data declarations, constructors, and record fields and bringing them over to the generated TS. You can see what that looks like here.

Now that we can work with Haddocks, we can write annotations in them that change the behavior of TS generation. In 4c587754358dad3463ae5e5dd46b4b334408348d, I added an annotation called @no-emit-typescript. When it appears on a constructor or record field, that thing will not be emitted from the formatTSDeclarations function. You can see this in action here.

I think this is a nice way of doing things which addresses the concerns I raised above. The only downside I see is you need a version of GHC/template-haskell which contains getDoc, the earliest of which seems to be 2.18.0.0/GHC 9.2.

What do you think?

[SamProtas]

Hey @thomasjm very cool!

This solves our problem and I'd be happy to pull in that update.

Just curious, how does this handle malformed haddocks? If there's an error elsewhere in the module, does this still succeed? If the field haddocks are malformed, does the template haskell fail at compile time, or is the haddock Nothing and ignored? Ultimately I think either is fine, I just know haddocks can be tricky to get right, and in the past there's been work done to turn off haddocks for dependencies to avoid build failures.

[thomasjm]

Just curious, how does this handle malformed haddocks?

Well, you definitely need to pass the -haddock GHC flag when compiling or else the comments aren't available.

The docs of -haddock say the following:

With this flag GHC will parse Haddock comments and include them in the interface file it produces... Note that this flag makes GHC’s parser more strict so programs which are accepted without Haddock may be rejected with -haddock.

I'm not sure if the set of rejected programs from this is the same set that will fail if you actually run Haddock.

Also TIL when looking at the GHC manual about -Winvalid-haddock, which turns malformed haddocks into warnings.

[thomasjm]

You want to try out current master branch and let me know how it works? Once we're happy with it I can polish it up and cut a release.

[thomasjm]

I just released this as part of 0.5.0.0, so I'm going to close this for now. Feel free to hit me up if you need any tweaks.