Fixed: TypeScript compile error #11
Conversation
|
Thanks for the PR, I'm looking into it. |
There was a problem hiding this comment.
Thanks for the addition of this signature. You are right that it was missing. I was trying to find a way to avoid adding a new signature but due to the relationship between the arguments and result you have to add one.
A better return type would be PluginError<E | {[K in keyof E]: undefined}>. It helps with inference of custom errors.
Please update the return type and add the following test case below yours:
{
// Inference with union type on second parameter
const PLUGIN_NAME = "test";
interface FooError extends Error {
foo: number;
}
function createPluginError(err: FooError | string) {
return new PluginError(PLUGIN_NAME, err);
}
const fooError: FooError = Object.assign(new Error("something broke"), {foo: 1});
const pluginError = createPluginError(fooError);
if (pluginError.foo !== undefined) {
const foo: number = pluginError.foo;
}
}Note: The current Typescript's version treats the return type above as PluginError<Partial<E>> but if the engine improves it may pick the relationship that either it has all of E's custom keys or none (not partial). With this kind of engine, the following test would pass. I'm interested if you have a solution with the current version:
{
// Inference with union type on second parameter and dependent properties
const PLUGIN_NAME = "test";
interface FooBarError extends Error {
foo: number;
bar: number;
}
function createPluginError(err: FooBarError | string) {
return new PluginError(PLUGIN_NAME, err);
}
const fooError: FooBarError = Object.assign(new Error("something broke"), {foo: 1, bar: 2});
const pluginError = createPluginError(fooError);
if (pluginError.foo !== undefined) {
const bar: number = pluginError.bar; // Error: `number | undefined` is not assignable to `number` (the existance of `foo` is not used)
}
}But it's more a "nice-to-have" thing: at this point you should just cast your value.
3d9c03f to
356ecf5
Compare
|
Regarding the actual change in the types, you are right that you can in fact just add a union in the signature because all the plugin errors extend Just replace new (plugin: string, message: string, options?: Options): PluginError;
new <E extends Error>(plugin: string, error: E, options?: Options): PluginError<E>;by: new <E extends Error = Error>(plugin: string, error: E | string, options?: Options): PluginError<E>;and add a test case, this should be enough for this PR. |
|
done. |
|
|
||
| const fooError: IFooError = Object.assign(new Error('something broke'), {foo: 1}); | ||
| const pluginError = createPluginError(fooError); | ||
| const foo: number = pluginError.foo; |
There was a problem hiding this comment.
Thanks again for the change and sorry for the back-and-forth.
At this point, foo should be number | undefined: if you invoke createPluginError("message"), then foo won't be defined. This was the reason why in my first message I said that an additional signature was required.
I then got confused in my reply two hours ago when I told you to merge the signatures: I should have stuck with my initial reply consisting in just updating the return type of your initial commit.
export interface Constructor {
/**
* @param options Options with plugin name and message
*/
new(options: Options & {plugin: string, message: string}): PluginError;
/**
* @param plugin Plugin name
* @param message Error message
* @param options Error options
*/
new (plugin: string, message: string, options?: Options): PluginError;
/**
* @param plugin Plugin name
* @param error Base error
* @param options Error options
*/
new <E extends Error>(plugin: string, error: E, options?: Options): PluginError<E>;
/**
* @param plugin Plugin name
* @param error Base error or error message
* @param options Error options
*/
new <E extends Error = Error>(plugin: string, error: E | string, options?: Options): PluginError<E | {[K in keyof E]: undefined}>;
/**
* @param plugin Plugin name
* @param options Options with message
*/
new(plugin: string, options: Options & {message: string}): PluginError;
}Here is the explanation:
// Nitpick: The convention in TS is to avoid the `I` prefix for interfaces.
interface FooError extendsError {
foo: number;
}
function createError1(error: FooError) {
const pluginError = new PluginError(PLUGIN_NAME, error);
// In this case, we know that the second parameter is an error with a custom property `foo`
// So the result will also have a property `foo`
const foo: number = pluginError.foo;
}
function createError2(error: string) {
const pluginError = new PluginError(PLUGIN_NAME, error);
// In this case, we know that the second parameter is a string. It means that the result is a simple `PluginError`
// and does not have `foo`. The following code would cause a compilation error:
// const foo: any = pluginError.foo;
}
function createError3(error: FooError | string) {
const pluginError = new PluginError(PLUGIN_NAME, error);
// In this case we don't know if we received an error instance or string: we cannot use the type-checker
// to prove that `foo` is defined. The correct type for the `foo` property is `number | undefined`.
const foo: number | undefined = pluginError.foo;
// It will be defined if we call `createError3(Object.assign(new Error("msg"), {foo: 1}));`
// It will be undefined if we call `createError3("msg");`
}The FooBar case is not yet properly supported by the engine so it's better to not test it. Here is a better illustration of the problem:
interface Named {
name: string;
foo: undefined;
bar: undefined
}
interface NamedFooBar {
name: string;
foo: number;
bar: number;
}
type Union = Named | NamedFooBar;
function exec(union: Union) {
if (union.foo !== undefined) {
// This fails with TS2322 when `strictNullChecks` is enabled.
const bar: number = union.bar;
}
}|
@gucong3000 Edit: We also should support case such as |
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 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
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 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
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
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
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 #10 and #11 for details.
- Closes #10
- Closes #11
/cc @gucong3000
Closes #10