Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule idea: enforce "correct" branch() typing via type-aware rule? #58

Open
helixbass opened this issue May 14, 2021 · 8 comments
Open

Comments

@helixbass
Copy link
Owner

As discussed with @peterstuart, it seems maybe impossible to truly correctly type branch()/branchPure()/renderNothing()/returns() (in the context of flowMax()'s typing)

But it seems feasible to enforce that eg you don't return something unexpected from a branch() + returns() via a type-aware ESLint rule that eg looks for uses of returns() and compares its "return type" against the calculated return type of the enclosing flowMax() (and flags if they disagree)?

@peterstuart
Copy link
Contributor

This seems tough, because in the case of ad-hok helpers which may be built with a flowMax, early returns wouldn’t actually have the same return type as the “real” return type. Eg. you might early return a loading indicator if data isn’t present, but the final step in the flowMax might just return some props, expecting to be used in a containing flowMax later.

@helixbass
Copy link
Owner Author

Ah good point - I guess that's sort of because of the "hoisting" magic of nested flowMax() (that ad-hok helpers doing return() "return" from their parent chain, not just their own flowMax())?

Since we're in ESLint-world, could bake in naming-convention-awareness and for ad-hok helpers either disable this rule or have it assume that it would be included in a React component in which case we're dealing with a known return type (ReactElement<any, any> | null or whatever the return type of FC is exactly)?

@peterstuart
Copy link
Contributor

So use the presence of (specific?) ad-hok helpers as a clue that you are in React-land? That might work. I think we'd have to try it and see if in practice there were too many false positives.

This feels like it is begging for a type-level solution, but it's hard to imagine one that doesn't make the ergonomics of ad-hok way worse. Like we were talking about earlier, this feels "monad-y" to me, but without the syntactic sugar that Haskell/etc. provide for you for monad operations, I think it'd feel very heavyweight.

@peterstuart
Copy link
Contributor

peterstuart commented May 14, 2021

I'm not sure if this will be comprehensible without being familiar with Haskell, but the way something like that would look there would be:

userComponent = do
  user <- loadUser
  let firstName = user.firstName
      lastName = user.lastName
  _ <- redirectIfNotAuthorized user -- return value unused, so _
  h1 (firstName + " " + lastName)

The lines with <- are allowing "monad-y" sorts of things to happen (eg. early returns), and the rest of the do-block only executes if they don't early return. The right-hand side is the function being called, and the left is the non-early return result of that function. The other lines are "normal" stuff.

The syntax is basically just writing a bunch of flatMaps for you, that would be harder to read if they were written normally. I'm not sure how something like that could be reproduced in TS, but that might be an interesting experiment.

@helixbass
Copy link
Owner Author

So use the presence of (specific?) ad-hok helpers as a clue that you are in React-land?

No what I meant is that if we're basically starting from seeing a returns()/renderNothing() and then trying to "backtrack" to whether its "return type" is type-safe or not, you would I guess look for the const addSomething = flowMax(...) naming of its enclosing flowMax()/declaration. And if that name is add*, assume that it's a React-land ad-hok helper

But ya there could likely be other clues eg in the body of the flowMax() that it's a React-land flowMax()

Actually another way to distinguish could be that since you basically only use returns()/renderNothing() inside of a branch(), that tells you directly that it's React-land because branch() bakes in React-specific stuff, you have to use branchPure() for non-React-component ad-hok "branches"

In practice there are things that don't fit these patterns though, eg ad-hok-utils's branchIfNullish():

branchIfNullish('someProp', {returns: () => <ThisShouldBeTypeChecked />}),

But this approach is fundamentally a "patch" so might as well start with the most common cases and then refine the patching further

@helixbass
Copy link
Owner Author

I'm not sure if this will be comprehensible without being familiar with Haskell

I'm a couple steps away from being able to follow this

This feels like it is begging for a type-level solution, but it's hard to imagine one that doesn't make the ergonomics of ad-hok way worse

Ya it's unlikely that I'd want to change the ergonomics of ad-hok but still find it interesting to try and wrap my head around the patterns

@helixbass
Copy link
Owner Author

Ok started playing with a type-aware ESLint rule on the iss58-no-incompatible-branch-types branch

I've figured out how to extract what appears to be a correct return type of a returns() callback from the Typescript type checker. But the thing I'm running into now is that I want to basically ask Typescript (at the type level) whether that type is "compatible with" (ie could be assigned to/is a subset of) the known return type of the enclosing flowMax(). I found this answer, but it's describing a technique of generating a new dummy Typescript file and then typechecking it, which sounds like a lot of overhead. It seems conceptually like there should be some helper inside the Typescript type checker that you can pass two types to and ask if one is assignable to the other?

@helixbass
Copy link
Owner Author

Ok it seems clear that this is not exposed by the Typescript type checker API: microsoft/TypeScript#9879 / microsoft/TypeScript#9943. But might be worth checking out the ts-simple-type library mentioned there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants