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

Update types to support union-type arguments #13

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

demurgos
Copy link
Member

@demurgos demurgos commented Jan 23, 2018

This commit updates the signature of the PluginError constructor to loosen the constraints on the parameters in order to support union types while keeping inference on custom error properties.
This is a semver minor update (fix)

This allows for example to use the pattern below:

function createPluginError(error: Error | string) {
  return new PluginError("test", error);
}

(The tests were updated with more complex cases)

See the discussions in #10 and #11 for details.

/cc @gucong3000

This commit updates the signature of the `PluginError` constructor to
loosen the constraints on the parameters in order to support union types
while keeping inference on custom properties.
This is a semver minor update (fix).

This allows for example to use the pattern below:
```
function createPluginError(error: Error | string) {
  return new PluginError("test", error);
}
```
(The tests were updated with more complex cases)

See the discussions in gulpjs#10 and gulpjs#11 for details.

- Closes gulpjs#10
- Closes gulpjs#11

/cc @gucong3000
@phated
Copy link
Member

phated commented Jan 23, 2018

@demurgos do you have write permissions here? I don't understand all this stuff so I'll leave it up to you to merge. Let me know when I should publish.

@demurgos
Copy link
Member Author

demurgos commented Jan 23, 2018

I only have write permission to plugin-error (Edit: gulp-util) for the moment.

The main issue was that the types defined multiple possible signatures for the constructor, but each of those required you to know the exact type of the value. If you did not know the exact type, it failed even if any combination worked.

Here is a simpler example:

/**
 * This interface defines a function that has two signatures:
 * - Accept a number
 * - Accept a string
 * This is less-general (sub-type) than `LooseHandler`
 */
interface StrictHandler {
  (value: number): void;
  (value: string): void;
}

/**
 * This interface defines a function that has a single signature:
 * - Accept a number or string
 * This is more general (super-type) than `StrictHandler`;
 */
interface LooseHandler {
  (value: number | string): void;
}

// Define functions with these types:
const strictHandler: StrictHandler = console.log;
const looseHandler: LooseHandler = console.log;

// Define some values:
const num: number = 1;
const str: string = "foo";
const numOrStr: number | string = Math.random() < 0.5 ? 2 : "bar";

// When you know the exact types, you can use any handler:
strictHandler(num);
strictHandler(str);
looseHandler(num);
looseHandler(str);

// The difference is with numOrStr. Even if `StrictHandler` can support any value of `number | string`,
// you cannot call it because the signatures are "disjoint". On the other hand, `LooseHandler` has a
// unified signature that lets TS know that it's safe to pass this value even if you don't know its exact
// type
looseHandler(numOrStr);
// strictHandler(numOrStr); // Compilation error

This PR shuffles the signatures to maximize the use of union types while retaining the most inference possible and keeping validity. Validity here means that any program that compiles will not encounter any type-related error at runtime. This usually means that some programs with a valid runtime will be rejected by the compiler because it cannot reason about them. The issue #10 and the example above demonstrates one of those false-positives: the programs are valid at runtime but are rejected by the compiler. This issue reduces the number of false-positive and lets more programs with a valid runtime pass compilation.

@phated
Copy link
Member

phated commented Jan 23, 2018

@demurgos 👍 Also, I think you meant gulp-util - I've added this repo to your team.

@demurgos demurgos merged commit eae2f4c into gulpjs:master Jan 23, 2018
@demurgos demurgos deleted the issue-10 branch January 23, 2018 22:56
@demurgos
Copy link
Member Author

It should be good to publish as v1.0.1.

@phated
Copy link
Member

phated commented Jan 23, 2018

Published. Nice work @demurgos

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.

TypeScript compile error
2 participants