Handle missing properties correctly #434
Comments
|
const x: { a: unknown } = { a: undefined } // okhowever this is a bug const T = t.type({ a: t.unknown })
assert.strictEqual(T.is({}), true)because the property |
|
Yes. Sorry. I accidentally submitted the bug when I was still typing. I think it is stated correctly now. |
|
It turns out the bug isn't limited to The current implementation allows for a property to be missing if the type of that property allows for In TypeScript, Here are some examples that illustrate the difference and serve as a reference for the expected behavior: const a1: { a: undefined } = { a: undefined } // ok
const b1: { a: undefined } = {} // error
const c1: { a: undefined } = { a: 1 } // error
const d1: { a?: undefined } = { a: undefined } // ok
const e1: { a?: undefined } = {} // ok
const f1: { a?: undefined } = { a: 1 } // error
const a2: { a: number } = { a: undefined } // error
const b2: { a: number } = {} // error
const c2: { a: number } = { a: 1 } // ok
const d2: { a?: number } = { a: undefined } // ok
const e2: { a?: number } = {} // ok
const f2: { a?: number } = { a: 1 } // ok
const a3: { a: number | undefined } = { a: undefined } // ok
const b3: { a: number | undefined } = {} // error
const c3: { a: number | undefined } = { a: 1 } // ok
const d3: { a?: number | undefined } = { a: undefined } // ok
const e3: { a?: number | undefined } = {} // ok
const f3: { a?: number | undefined } = { a: 1 } // okAlso note that const a: void = undefined; // ok
const b: unknown = undefined; // ok
const c: null = undefined; // error
const d: string = undefined; // error
const e: number = undefined; // error
const f: number[] = undefined; // error
const g: 'literal' = undefined; // error
const h: {} = undefined; // error
const i: { name: string } = undefined; // error
const j: null | undefined = undefined; // okAnyway, I've completed the fix and added a bunch of additional tests to make sure everything behaves as expected. I also modified a few existing tests that were expecting missing properties to be allowed where they shouldn't be. The fix is only a few lines. I'm thinking I might have added more tests than I need so I'll review the tests in the morning to see what tests I can safely eliminate before submitting a pull request. |
A property can only be missing if the property is optional. Properties with types that accept undefined values must always be present unless they are optional properties.
leilapearson
changed the title
mmmh this is unexpected, this bug should involve the |
|
@gcanti Here's a list of test cases that fail without the fix: I checked the tests against what the TypeScript compiler does in the equivalent cases, so I think they're real failures... Let me know if I missed something though. **Edit ** : I removed some test cases from the above list because they weren't cases where decode succeeded but should have failed. The tests were failing with the old code and the new tests only because the decode errors were different - not because the behavior was wrong. |
|
@leilapearson The following fix should be enough // `is` definition in `type` combinator
(u): u is { [K in keyof P]: TypeOf<P[K]> } => {
if (UnknownRecord.is(u)) {
for (let i = 0; i < len; i++) {
const k = keys[i]
const uk = u[k]
if ((uk === undefined && !hasOwnProperty.call(u, k)) || !types[i].is(uk)) {
return false
}
}
return true
}
return false
} |
|
Hmm... @gcanti Wouldn't it be better to align with the typescript compiler? I have the whole fix ready and fully tested. It's only 4 lines of actual code in t.type. I've added lots of additional test code though :-) The fix only impacts failure cases - meaning things that were expected to fail that didn't. This is based on the typescript compiler but also makes sense logically based on the definition of optional properties versus properties with undefined values.
Of course this will be a breaking change for anyone relying on the old behavior - including anyone who happens to expect the current behavior of properties with unknown types. I guess the fix as I've written it can only go into the 3.x release... If you like, I can write a version of the fix that allows people to opt in to the new behavior in 2.x too... ? e.g. for the 2.x branch we could have two versions of Not sure what I'd name the newer t.type ... Thoughts? |
No, import { either } from 'fp-ts/lib/Either'
const NumberFromString = new t.Type<number, string, unknown>(
'NumberFromString',
t.number.is,
(u, c) =>
either.chain(t.string.validate(u, c), s => {
const n = +s
return isNaN(n) ? t.failure(u, c, 'cannot parse to a number') : t.success(n)
}),
String
)
console.log(NumberFromString.decode('1')) // => right(1)
console.log(NumberFromString.is('1')) // falseHowever const T = t.type({ a: t.unknown })
assert.strictEqual(T.is({}), true) |
|
Well the fix is in I do see your point about parsing though. It's true that custom types can be written to parse from anything to anything else and that's good and highly useful. That said, I still think it makes sense - and would be expected by most people - for the basic types in People who create custom types are free to do as they wish - including turning strings into other things which is certainly a common case. Nothing about this fix prevents that... It's likely I'm still missing something though, so putting my curiosity hat on...
Thanks for your help as always. |
|
@gcanti I'll submit the pull request now so you can take a look as it. I can always update the PR with whatever changes make sense. Regarding the idea of this being a breaking change, I think it's more of a bug fix - but in this case people may have been inadvertently relying on the bug behavior. I don't think it's entirely black and white what to do from a semver perspective for cases like this... Getting ahead of myself perhaps, but if you do decide to take the change -- which I hope you do since I spent 2 solid days testing it! -- I would be happy to write something up for people on how to migrate to the new behavior if you think that would help. |
I suggest to discuss them separately, I'll gladly accept a PR containing only a fix for IIRC the current behaviour of shouldn't fail. Example const T = t.type({ a: t.undefined })
console.log(T.decode(JSON.parse(JSON.stringify(T.encode({ a: undefined }))))) // right({ a: undefined })
The name |
|
Ah... @gcanti the extra context helps a lot. JSON indeed can't directly represent undefined values and strips out those properties in That said, you can use a replacer function in Then, when parsing, you can use a reviver function in As you can see by the stack overflow question, this is something that people are used to dealing with since it's just how JSON works. Anyway, I really don't think it should be up to For example, let's say I have a data source that sends me both Users and HomelessUsers. Users must have an address property, but the address value can be undefined. HomelessUsers must not have an address property. That's how I expect to distinguish between them. Let's also say I've already parsed the data to the correct object representations -- or maybe I didn't need to parse the data because my data source is creating and sending me objects directly in the same process. Here's some sample code: import * as t from 'io-ts';
import { isLeft, fold } from 'fp-ts/lib/Either';
import { identity } from 'fp-ts/lib/function';
import * as prettyFormat from 'pretty-format';
const User = t.type({
name: t.string,
address: t.union([t.string, t.undefined]),
});
type User = t.TypeOf<typeof User>;
const HomelessUser = t.type({
name: t.string,
});
type HomelessUser = t.TypeOf<typeof HomelessUser>;
const userData = [{ name: 'Ann', address: undefined }, { name: 'Joe' }];
const eitherAnn = User.decode(userData[0]);
const eitherJoe = User.decode(userData[1]);
console.log(`eitherAnn is a ${isLeft(eitherAnn) ? 'left' : 'right'}`);
console.log(`eitherJoe is a ${isLeft(eitherJoe) ? 'left' : 'right'}`);
console.log(`eitherAnn is ${prettyFormat(fold(identity, identity)(eitherAnn))}`);
console.log(`eitherJoe is ${prettyFormat(fold(identity, identity)(eitherJoe))}`);
// OUTPUT:
// eitherAnn is a right
// eitherJoe is a right
// eitherAnn is Object {
// "address": undefined,
// "name": "Ann",
// }
// eitherJoe is Object {
// "address": undefined,
// "name": "Joe",
// }Note that according to the data, Ann is a When I used Now, maybe we fix If I'm used to how Typescript types behaves, then I'm sure to be confused... By the way, I mainly use
|
Actually, after some more investigation, this is a regression introduced in the last commit, @leilapearson I'll put up a PR for this and release version 2.1.3. |
|
@gcanti I merged your latest into my branch and updated the pull request. In my branch, In master,
So... have you given this behavior any more thought? I still think it is confusing to have these two exceptions, and that it's confusing to magically create properties that don't exist. Codecs in general can certainly do such things, but That said, I remain curious and I'd love to hear thoughts and opinions from anyone in the community. |
|
Nit: a third exception is My two cents: making In my opinion, it also shouldn't go into the next version, because decoding to undefined is a useful feature, and an unnecessarily disruptive change, even for a major version. When a property is One note - I agree that there being a difference between A solution to the above problem I've suggested in the past is this PR, which introduces an |
|
Thanks @mmkal. Yes, I didn't mention I was also thinking that something like a type desiredType = {
a: string;
b?: string;
c: string;
};
const withPartial = t.intersection([
t.type({
a: t.string,
c: t.string,
}),
t.partial({
b: t.string,
}),
]);
const withOptional = t.type({
a: t.string,
b: t.optional(t.string),
c: t.string,
});While I like the simplicity of the syntax for The syntax below would be pretty intuitive too: const WithQuestionMark = t.type({
a: t.string,
'b?': t.string,
c: t.string,
});BUT... If anyone has a property key that ends in a question mark that shouldn't be optional, that would cause problems - so I definitely wouldn't be in favour of this as a default behavior. Adding a Anyway, putting aside for the moment the possibility of adding a new syntax for optional properties -- since the functionality can be accomplished today (albeit a little awkwardly) -- two options for adding #435 behavior but in a non-breaking way would be:
|
|
@mmkal - just a bit of clarification on your point:
If people can't use a custom replacer, presumably they still do have the ability to specify their desired post-decode type so that the property is optional? And therefore there is no problem in the property not being in the JSON? If they do need the property to be added with undefined (or null - or some arbitrary default value) if not present, then presumably they could add the property themselves post-decode with a simple transformation? Just asking. Obviously this would be less convenient in these cases than the current behavior and I'm not suggesting it's a better alternative. I'm just wanting to confirm it's a possible alternative to consider. |
|
Thinking a bit more on this, I realized that my mental model for codecs is that they either act as validated type aliases or they act as transcoders. A codec that acts as a trancoder:
A codec that acts as a validated type alias:
Since So far everything makes sense. What doesn't make sense is that |
|
@leilapearson a codec is basically a
here's the translated
where the actual meaning of Now const u: any = {}
const a = { a: undefined }
console.log(u['a']) // => undefined
console.log(a['a']) // => undefinedso when we are using If you really care about optional properties, you can use |
console.log(`a === u is ${a === u}`);
console.log(`a == u is ${a == u}`);
// Output:
// a === u is false
// a == u is falseIt's true that the value of a missing property is I don't think import { equals } from 'expect/build/jasmineUtils'; // same equals function as used by jest
// 1. pipe(codec.decode(u), E.fold(() => u, codec.encode)) = u for all u in unknown
// 2. codec.decode(codec.encode(a)) = E.right(a) for all a in A
const input = {};
const codec = t.type({ a: t.undefined });
const decoded = codec.decode(input);
const decodedValue = fold(t.identity, t.identity)(decoded) as t.TypeOf<typeof codec>;
const reEncoded = codec.encode(decodedValue);
const options = { min: true };
const print = (value: any) => prettyFormat(value, options);
console.log(`input = ${print(input)}`);
console.log(`decodedValue = ${print(decodedValue)}`);
console.log(`reEncoded = ${print(reEncoded)}`);
console.log(`equals(reEncoded, input, undefined, true) is ${equals(reEncoded, input, undefined, true)}`);
// Output:
// input = {}
// decodedValue = {"a": undefined}
// reEncoded = {"a": undefined}
// equals(reEncoded, input, undefined, true) is falseEdit: |
|
Hi again @gcanti, Just to be thorough I tried a similar test using prisms from I also fixed my object comparison in the code above. You are correct that if you compare the objects using However, if you compare by using the results of a Presumably the laws should hold both for a Or am I misunderstanding or doing something wrong? Thanks for your patience and time on this. |
@leilapearson the meaning of
"incorrect" is not the right word, you can derive a "correct" In general strict equality is useless (except for primitive types) because many functions would be unexpectedly impure import { eqStrict } from 'fp-ts/lib/Eq'
interface Person {
name: string
}
// is this function pure?...
function makePerson(name: string): Person {
return { name }
}
// ...no, IF we use `eqStrict` as equality for `Person`
console.log(eqStrict.equals(makePerson('foo'), makePerson('foo'))) // => falseA more sensible equality would be: "two That's the equality you get from import { getStructEq, eqString } from 'fp-ts/lib/Eq'
interface Person {
name: string
}
// is this function pure?...
function makePerson(name: string): Person {
return { name }
}
// ...yes
console.log(getStructEq({ name: eqString }).equals(makePerson('foo'), makePerson('foo'))) // => trueBack to our problem, we can just assume that the following holds for example using the following import { Eq, fromEquals } from 'fp-ts/lib/Eq'
const eq: Eq<{ a?: unknown } | { a: unknown }> = fromEquals((x, y) => x.a === y.a)
console.log(eq.equals({}, { a: undefined })) // => true |
|
Hi @gcanti We're talking about a different type of strict equals. The jest version of Using the jest version of import { equals } from 'expect/build/jasmineUtils'; // same equals function as used by jest
export const runEqualsExample = () => {
const strictEquals = (a: any, b: any) => equals(a, b, undefined, true);
interface Person {
name: string;
}
function makePerson(name: string): Person {
return { name };
}
console.log(equals(makePerson('foo'), makePerson('foo'))); // true
console.log(strictEquals(makePerson('foo'), makePerson('foo'))); // true
console.log(equals({}, { a: undefined })); // true
console.log(strictEquals({}, { a: undefined })); // false
};The documentation for the jest version of
The key point here is that while In a library where type safety is a major goal, I was expecting that the jest version of |
|
P.S. I also just realized that one of the main reasons that it's helpful to create properties when their values are undefined is that it's easier and more readable to do this: export const aType = t.type({
a: t.union([t.string, t.undefined]),
b: t.string,
})than it is to do this: export const aType = t.intersection([t.partial({ a: t.string}), t.type({ b: t.string})]); The resulting types are more readable too. If it was just as easy to specify optional properties as optional values - as @mmkal was pushing for with PR #266 - then people probably wouldn't rely on this behavior so much. |
|
Hi @gcanti. I see you closed this, and probably I should just take the hint - but I'm honestly only trying to help improve what I think is a very valuable and important library. This is not about my use of the library personally. At this point I know how For example, say I have these types: type User = { name: string; address: string | undefined };
type HomelessUser = { name: string };
type AnyUser = User | HomelessUser;
Now, I know I can't discriminate between the types in this sum type by checking the value of the address field. It might be undefined in one, and it will always be undefined in the other. So in code that needs to discriminate, I check if the property is present or absent instead. You'd be right to argue that adding a tag to my types would be a better approach, but everything works fine with this approach too. Next I decide I should switch to I create what I think are the equivalent types using const User = t.type({
name: t.string,
address: t.union([t.string, t.undefined]),
});
type User = t.TypeOf<typeof User>; // { name: string; address: string | undefined; }
const HomelessUser = t.type({
name: t.string,
});
type HomelessUser = t.TypeOf<typeof HomelessUser>; // { name: string }
const AnyUser = t.union([User, HomelessUser]);
type AnyUser = t.TypeOf<typeof AnyUser>; // { name: string; address: string | undefined; } | { name: string }Unfortunately, once I introduce type EncodedAnyUser = ReturnType<typeof AnyUser.encode>;
// { name: string; address: string | undefined; } | { name: string }
const userData: EncodedAnyUser[] = [{ name: 'Ann', address: undefined }, { name: 'Joe' }];
console.log('Given this user data:');
console.log(`${pretty(userData[0])}`); // {"address": undefined, "name": "Ann"}
console.log(`${pretty(userData[1])}`); // {"name": "Joe"}
console.log('Send info about shelters to any users who do not have an address property');
processUsers(userData);
// no info sent to Ann with data {"address": undefined, "name": "Ann"} -- Good
// no info sent to Joe with data {"address": undefined, "name": "Joe"} -- Oops! Anyway, that's the crux of it for me. By using I'll be quiet now. |
🐛 Bug report
Current Behavior
Expected behavior
Similarly, in vanilla TypeScript:
Reproducible example
See above
Suggested solution(s)
Will submit a pull request.
Additional context
Your environment
The text was updated successfully, but these errors were encountered: