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

fix wrong behaviour with respect to TypeScript's type system #9

Closed
wants to merge 2 commits into from

Conversation

gcanti
Copy link
Owner

@gcanti gcanti commented Feb 10, 2017

This PR aims to fix some current wrong behaviours with respect to TypeScript's type system (feedback much appreciated)

New Feature

  • add support for jsnext

Breaking Changes

  • t.Object type. Renamed to t.Index, now accepts arrays so is fully equivalent to { [key: string]: any }.
  • t.instanceOf combinator. Removed.
  • t.object / t.record combinator. Renamed to t.interface. ObjectType / RecordType to InterfaceType. Excess properties are now checked:
    const Person = t.interface({ name: t.string })
    console.log(t.validate({ name: 'Giulio', a: 1 }, Person)) // => Left(...)
  • mapping combinator. Renamed to index. MappingType to IndexType.
  • intersection combinator. Due to the new excess property checks in t.interface now only accept InterfaceTypes.

/cc @dmorosinotto

@gcanti gcanti added this to the 0.1 milestone Feb 10, 2017
@gcanti
Copy link
Owner Author

gcanti commented Feb 10, 2017

Excess properties are now checked

Another option (which I like much more) is to prune unknown properties instead of failing.

Example

const Payload = t.interface({
  a: t.string,
  b: t.number
})

const bigPayload = { a: 's', b: 1, c: true /* other props here...*/ }

// the validation here doesn't fail and the unknown properties are removed
// hence the result honours the contract
t.validate(bigPayload, Payload).map(p => console.log(p)) // => { a: 's', b: 1 }

This would allow for partial descriptions of APIs with big payloads.

A candidate implementation is on branch whitelist

@gyzerok
Copy link

gyzerok commented Feb 13, 2017

@gcanti any plans regarding merge estimation?

Another option (which I like much more) is to prune unknown properties instead of failing.

I do really need this feature as well. Working right now with legacy APIs and most of the data make no sense to me, so it feels inefficient and misleading to write types for all the pieces.

@gcanti
Copy link
Owner Author

gcanti commented Feb 13, 2017

@gyzerok In order to quickly merge one of these PRs I'd love some reviews / feedback. In particular there are 3 possible choices with respect to unknown props

  • current behavior (v0.0.2): type check known props, copy unknown props
  • this PR: type check known props, fail with unknown props
  • whitelist branch: type check known props, prune unknown props

(and then intersection behavior follows)

t.instanceOf combinator was removed because TypeScript type system is structural (my fault, this library is a porting of flow-io and Flow contains nominal types but here it doesn't make sense)

t.object is renamed to t.interface because object is a new official type in 2.2 and using an interface as example is the easiest way to explain how (the ex) t.object works

t.mapping -> t.index and t.Object -> t.Index bacause in the TypeScript documentation the type { [key: string]: any } is called "index signature" (if I'm not wrong)

(there are a couple of other PRs for the 0.1 milestone)

@gyzerok
Copy link

gyzerok commented Feb 13, 2017

t.instanceOf

Well, not really can say something here. I do not have any use case where I need to check for instance of.

t.object -> t.interface

This renaming makes a lot of sense to me and I'd say that it's probably best way to rename it out of all possible ones.

t.mapping -> t.index

Here it seems like new one and old one are both not really sensible. Just by looking at both names I can't clearly understand what the thing do. Maybe something like t.keyValuePairs or t.dictionary could be better? If one need to sacrifice fanciness of the API to achieve clearness it sounds like a good trade-off. Even though in TS docs it is called "index signature", how many people you expect are reading TS docs so attentively? For example I didn't know that :)

Unknown props behaviour

Out of all possible options here I'd say that pruning unknown props is better one:

  1. If you use API which have much more data then you need in your app then you don't want to define type bigger than necessary. If validation would fail in case of receiving more data then everyone will need to define types up to any details they don't need. Also semantically if data have more fields it doesn't make it "invalid". What do you think?
  2. Copying props may result in some possibilities to hack validation a bit, which is not nice. For example somewhere you just degrade to any (something as any) and access some field which does not exist in you type signature, but exists on data. So in my opinion we should prevent people as much as possible from this sort of issues.

@gcanti
Copy link
Owner Author

gcanti commented Feb 13, 2017

t.dictionary

I like it (also is the name I'm already using in tcomb)

Also semantically if data have more fields it doesn't make it "invalid". What do you think?

It depends. Flow by default allows for additional props and you can turn on strictness with the $Exact<T> type (or the {||} syntax).

TypeScript seems the other way around: strict by default

const x: { name: string } = { name: 'Giulio', a: 1 } // error: Object literal may only specify known properties, and 'a' does not exist in type '{ name: string; }'.

and then you can loose the type

const x: { name: string, [key: string]: any } = { name: 'Giulio', a: 1 } // no error

(btw I vote for pruning as well)

@gyzerok
Copy link

gyzerok commented Feb 13, 2017

I guess in TypeScript they are not strict in the same sense as in Flow. They behave more like extensible records: "object has at least these fields". Not even sure I can imagine use case for "at most" variation :)

@gcanti
Copy link
Owner Author

gcanti commented Feb 13, 2017

To summarize, by default

  • Flow: "object has at least these fields"
  • TypeScript: "object has exactly these fields"

so

const x: { name: string } = { name: 'Giulio', a: 1 }

is not an error in Flow but is an error in TypeScript.

Hence the current behaviour (v0.0.3) is not correct, either we must check for excess properties and fail or prune them.

@gyzerok
Copy link

gyzerok commented Feb 13, 2017

Ah, you are right, I kind of messed type definitions and just distructuring in mind.

Anyway prune is cool :)

@dmorosinotto
Copy link
Contributor

Hi @gcanti I'm sorry for late response but I had very busy days...
These are my feedback:

  • t.object -> t.interface rename 👍
  • t.instanceOf delete 👍
  • t.mapping -> t.index rename 👎 I prefer the suggestion t.dictionary 👍 by @gyzerok

And finally about the Unknown props I don't like the the fail strict-check behaviour, I prefer to simply ignore those props and maybe keep it there (just copying it), or eventually prune it but based on a precise user intention: let's say having a t.interface(..) that keeps unknown props, and a t.interfaceStrict(..) that prune the unknown props.

@gyzerok
Copy link

gyzerok commented Feb 14, 2017

@dmorosinotto according to what we discussed around record strictness in TypeScript it might be misleading to copy props, because in this case type signature will contradict underlying data.

I would propose to make only t.interface and see feedback from people. If nobody will complain about it, then, probably, nobody need it. So there is no necessity to increase API surface until we actually need it :)

Also I guess it's a bit off-topic to this PR, but I would say that library responsibility goes over just validation. I'd probably call it more decoding. From this perspective pruning doesn't feel anyhow wrong or unexpected. I'll open an issue with better explanation regarding validation/decoding later, so we can discuss it separately.

@gcanti
Copy link
Owner Author

gcanti commented Feb 14, 2017

Fine. Closing in favour of #20

@gcanti gcanti closed this Feb 14, 2017
@gcanti gcanti removed this from the 0.1 milestone Feb 14, 2017
@gcanti
Copy link
Owner Author

gcanti commented Feb 14, 2017

I'll open an issue with better explanation regarding validation/decoding later, so we can discuss it separately

@gyzerok Interesting, please do. In the meanwhile just an observation: validate's signature allows for deserialization / decoding logics (by design)

(value: any, context: Context) => Either<Array<ValidationError>, T>;

It says that you will find a T in the right or a list of errors in the left. But value can be anything, not necessarily a T.

Here an example: deserializing an ISO string to a Date

const DateFromISOString = new t.Type<Date>(
  'Date',
  (v, c) => {
    const d = new Date(v)
    return isNaN(d.getTime()) ? t.failure<Date>(v, c) : t.success(d)
  }
)

console.log(t.validate('foo', DateFromISOString)) // => Left(...)
console.log(t.validate('2017-02-14T14:24:39.446Z', DateFromISOString)) // => Right(Date(2017, 01, 14))

@gyzerok
Copy link

gyzerok commented Feb 14, 2017

Interesting, please do. In the meanwhile just an observation: validate's signature allows for deserialization / decoding logics

Yes thats exactly what I am talking about. The way you can use library goes a bit beyond validation. And I'd personally say it is useful feature. I do already have date parsing in a way similar to your example in my code as well as a bit advanced number parsing (for example for "1 069,13").

Actually I've started using the library because I wanted something close to Elm decoders. For me it doesn't matter much if you work with raw JSON or parsed one, it was more about being able to achieve type safety.

I will write about decoding and overall experience soon, just need to find a bit of time to give thoughts a good shape.

@gcanti
Copy link
Owner Author

gcanti commented Feb 27, 2017

@gyzerok just a precisation with respect to my statement above

const x: { name: string } = { name: 'Giulio', a: 1 } is not an error in Flow but is an error in TypeScript

It holds only for Object literals

const y = { name: 'Giulio', a: 1 }
const x: { name: string } = { name: 'Giulio', a: 1 } // error
const z: { name: string } = y // NO error

@gcanti gcanti deleted the strict branch February 28, 2017 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants