jklmli / monapt

Options, Tries, and Futures for JavaScript / TypeScript
MIT License
172 stars 13 forks source link

Exclude null and undefined from Option()'s inferred type parameter #14

Closed theefer closed 7 years ago

theefer commented 7 years ago

With strict null checks enabled, the current Option() constructor causes the Option's inferred type to include _ | null | undefined if the argument was of such type. This is not intended, as the goal of Option is to map those two types to None, as is enforced by the constructor.

This patch ensures that T does not include such union types.

I've tried and failed to write a failing test to cover this change. By definition, it's not possible to assert on a parameterized type in JavaScript since generics are erased during the TypeScript compilation. I haven't found a pattern to do it in TypeScript either.

The closest I've found is a test like

it('extracts the type of the value, ignoring undefined', () => {
  var value = 'value' as string | undefined;
  var opt: monapt.Option<string> = monapt.Option(value);
  // ^ will cause tsc compilation to fail
});

run through tsc --noEmit for type checking. It will fail to compile, but only if strictNullChecks is enabled. However monapt currently has a number of existing errors when enabling strict null checks, so that'd need to be fixed first.

jklmli commented 7 years ago

Thanks!

theefer commented 7 years ago

Great, thanks for merging! Would there be any chance of doing a release with this fix? No big deal but I'm currently relying on an inline version of this patch, would be nice to use the pristine library instead :smile:

jklmli commented 7 years ago

Yup, can do - I'll push out a release later this weekend. Apologies for not being more on top of this, I plan to start doing weekly releases if needed.

And I'm happy to hear you're getting value out of monapt 👍

jklmli commented 7 years ago

Rebuilding doesn't seem to update the type definitions, so a new release would be equivalent to the current release.

I wonder if this is related to strict null checks. Any ideas?

EDIT: I think this may be a tooling issue, the definitions update correctly in the 1.0 branch. 😞

jklmli commented 7 years ago

Released in 0.7.1