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

[FE-2558] types for nullable arguments #651

Merged
merged 3 commits into from
Aug 8, 2022

Conversation

ptpaterson
Copy link
Contributor

@ptpaterson ptpaterson commented Aug 5, 2022

Notes

Jira Ticket

Makes arguments nullable for the following FQL functions

  • Let - in_expr can basically be anything.
  • IsNull - intuitively accepts null and returns true.
  • all other IsX functions - They all accept null.
  • Range - Indexes entries can have null values, so it is important to allow null, [null] and [SOMETHING, null] cursors.
  • Equals - accepts null values.
  • ToString - converts null to "null".
  • ContainsValue - can search for null in the items of an array or object's properties.

@henryfauna henryfauna merged commit 2133861 into v4 Aug 8, 2022
@henryfauna henryfauna deleted the FE-2558-types-for-nullable-arguments branch August 8, 2022 18:08
@cleve-fauna
Copy link
Contributor

Won't changing the types like this risk breaking existing customer code? @ptpaterson @henryfauna

@ptpaterson
Copy link
Contributor Author

If your current code is valid typescript, then you must be passing non-nullable types to the functions. Making the arguments nullable still allows non-null values, so there should not be any issues.

arguments to If were updated in Jan 2020 (a1fef50) in this same way. We have not had trouble with that.

@henryfauna
Copy link
Contributor

henryfauna commented Aug 8, 2022

@cleve-fauna Yeah to my understanding this should only be additive in terms of allowed types.

@cleve-fauna
Copy link
Contributor

cleve-fauna commented Aug 8, 2022

I think there could be some cases where someone is using one of these to curry or pass them as a parameter that we would risk breaking.

That's probably OK as long as we bump the minor version on the release and note that. e.g. 4.6.x ===> 4.7.0

You could verify that risk with a quick experiment.

@ptpaterson
Copy link
Contributor Author

ptpaterson commented Aug 11, 2022

I believe that we will not break in the case of currying the FQL functions.

If a function has a parameter of type ExprArg -> T, it is acceptable to provide a function of type (ExprArg | null) -> T because function types are are contravariant.

That is, while ExprArg is a subtype of (ExprArg | null), (ExprArg | null) -> T is a subtype of ExprArg -> T

So, if a user defines their own function type to mimic an FQL function, is doesn't matter if you pass in a function with the updated nullable arguments. The user function knows how to pass non-nullable arguments to the FQL function and that's okay. They cannot pass in null to their function, but that's already what they have -- we won't break anything.

/* *****************************************************************************
 * Non nullable fn types (matches signature before the update)
 */

type LetLikeFn = (vars: ExprArg, in_expr: ExprArg) => Expr
type EqualsLikeFn = (...args: ExprArg[]) => Expr


/* *****************************************************************************
 * curried functions that take FQL function types
 */

const CurryLetLike =
(fn: LetLikeFn) =>
(vars: ExprArg, in_expr: ExprArg) =>
  fn(vars, in_expr)

const CurryEqualsLike =
(fn: EqualsLikeFn) =>
(...args: ExprArg[]) =>
  fn(...args)

/* *****************************************************************************
 * Type tests using the updated query fns with nullable arguments
 */

CurryLetLike(q.Let) // <-- this line still compiles 🎉🎉🎉
  ({}, null)        // <-- Error: Argument of type 'null' is not assignable to parameter of type 'ExprArg'.

CurryEqualsLike(q.Equals) // <-- this line still compiles 🎉🎉🎉
  (1, null)               // <-- Error: Argument of type 'null' is not assignable to parameter of type 'ExprArg'.

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

Successfully merging this pull request may close these issues.

3 participants