Make notSetValue optional for typed Records

[morsecodist]

Tiny change to the type definitions.

First of all type-safe immutable records are glorious, thank you! I am a huge fan of what you are doing here. Secondly, your type declarations throw compiler errors for using the API as per the documentation. In your documentation for Record. You use get like so:

const { Record } = require('immutable@4.0.0-rc.9')
const ABRecord = Record({ a: 1, b: 2 })
const myRecord = new ABRecord({ b: 3 })

myRecord.get('a') // 1

Doing this with the current type definition produces the following error: Expected 2 arguments, but got 1

My new type definition will give you the following behavior:

const { Record } = require('immutable@4.0.0-rc.9')
const ABRecord = Record({ a: 1, b: 2 })
const myRecord = new ABRecord({ b: 3 })

myRecord.get('a') // Type: number
myRecord.get('a', 'a') // Type: number
myRecord.get('v', 'a') // Type: string
myRecord.get('v') // Error: 
// Argument of type '"v"' is not assignable to parameter of type '"a" | "b"'

This seems to capture the API pretty well.

Dicussion

With type safe records you could forbid the notSetValue entirely. Since we can force key to be a key of TProps we should never need a notSetValue. Using one in these cases will probably be unintentional. But this means there are ways to validly use the API in JS that you can't in TypeScript, which is understandable not to want.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[asazernik]

There are two other PRs (#1469, #1445) that address the same issue (#1463). This is the one that Does The Right Thing.

I suspect that the bug originated in a one-to-one translation between Flow and TypeScript - the former treats mixed and any parameters as optional, while the latter does not.

In addition to fixing that mistranslation, this also fixes an issue that is shared between the Flow and TS annotations - they didn't properly describe the behavior when a key is passed that's not in the Record. This is legal, and results in a different return type. @morsecodist, could you add a commit to this PR applying the same fix to immutable.js.flow? (And then remove the part of the comment right above the TS version that describes the bug 😛.)

[kozlitinaelja]

Thank you for fixing this!

[leebyron]

What if the key is set and the type of TProps[key] is different from the T value? Couldn't this result in a program that type checks but is incorrect?

[asazernik]

Do Records allow setting keys that are not in TProps?