codemix / babel-plugin-typecheck

Static and runtime type checking for JavaScript in the form of a Babel plugin.
MIT License
886 stars 44 forks source link

Improve legibility of the shape mismatch error #93

Closed gajus closed 8 years ago

gajus commented 8 years ago

Referring to the example from the documentation, https://github.com/codemix/babel-plugin-typecheck#shape-tracking.

type User = {
  name: string;
  email: string;
};

function demo (input: User): string {
  return input.name;
}

// demo({}); // TypeError
demo({name: 123, email: "test@test.com"}); // TypeError
// demo({name: "test", email: "test@test.com"}); // ok

The error is completely unreadable to the user:

/Users/gajus/Documents/dev/applaudience/showtime-app-event-agent/dist/createClient.js:9
    throw new TypeError("Value of argument \"input\" violates contract, expected User got " + (input === null ? 'null' : typeof input === 'object' && input.constructor ? input.constructor.name || '[Unknown Object]' : typeof input));
    ^

TypeError: Value of argument "input" violates contract, expected User got Object
    at demo (/Users/gajus/Documents/dev/applaudience/showtime-app-event-agent/dist/createClient.js:9:11)
    at Object.<anonymous> (/Users/gajus/Documents/dev/applaudience/showtime-app-event-agent/dist/createClient.js:16:1)
    at Module._compile (module.js:399:26)
    at Object.Module._extensions..js (module.js:406:10)
    at Module.load (module.js:345:32)
    at Function.Module._load (module.js:302:12)
    at Function.Module.runMain (module.js:431:10)
    at startup (node.js:141:18)
    at node.js:977:3

First of all,

demo({
    name: "test",
    email: "test@test.com"
});

is not an instance of User. The error ("expected User got Object") makes it look like the object needs to be constructed using User constructor function.

Therefore,

TypeError: Value of argument "input" violates contract. Input type Object does not match the shape of User type.

would be a good start.

gajus commented 8 years ago

The suggested phrasing works equally well regardless of input type or the type of the shape.

gajus commented 8 years ago

It would be nice to have a console error that would explain the mismatch, e.g.

{
    name: string, -> input type is number
    email: string
}

Though thats not straightforward. I wonder if there is an existing package that could be adapted for the purposes – diffing object values/ types.

phpnode commented 8 years ago

Totally agree but as you said, offering those detailed runtime errors is not so straightforward and will require refactoring the plugin a bit because right now we don't keep track of which particular check failed, we just generate something like:

if (!(typeof thing === "object" && typeof thing.foo === "function" && typeof thing.bar === "boolean")) {
  throw new TypeError("something is wrong!")
}

When what we really need is something like:

if (!(typeof thing === "object" && thing !== null)) {
  throw new TypeError("thing must be an object, got " + inspect(thing));
}
else if (!(typeof thing.foo === "function")) {
  throw new TypeError("thing.foo must be a function, got " + inspect(thing.foo));
}
else if (!(typeof thing.bar === "boolean")) {
  throw new TypeError("thing.bar must be a boolean, got " + inspect(thing.bar));
}

etc.

I'm pretty doubtful that an existing lib can be adapted for this, and we wouldn't gain much from it anyway. The important bit is that we retain control over where the error instance is created in order to preserve a sensible stack trace.

gajus commented 8 years ago

Totally agree but as you said, offering those detailed runtime errors is not so straightforward and will require refactoring the plugin a bit because right now we don't keep track of which particular check failed, we just generate something like:

Almost there https://github.com/gajus/pretty-print-object#annotate-value-types

gajus commented 8 years ago

See https://github.com/gajus/babel-plugin-typecheck-annotated-difference for an implementation example using pretty-print-object.

phpnode commented 8 years ago

@gajus like it but there are some problems to consider:

  1. How do we get the type definition object into the library considering the type does not exist at runtime?
  2. How does it behave with extremely large or nested objects or objects with e.g. very long string keys.
  3. How does it handle circular references?
  4. How do we ensure that the client code can import the pretty-print-object module? (the user might not have specified it in package.json)

I think the approach we'll have to take here will be a combination of the existing checks and some of the techniques from your library. I have some ideas for this, will PR them tonight.

gajus commented 8 years ago

How do we get the type definition object into the library considering the type does not exist at runtime?

That is a question for you.

There must be a way (at a babel plugin level) to get the type declaration before it has been stripped. We'd need it in a format of a regular object, e.g.

type FooType = {
    a: string,
    b: string,
    c: Array<string>
};

needs to be fed to my script in a form of:

{
    a: 'string',
    b: 'string',
    c: 'Array<string>'
};

How does it behave with extremely large or nested objects or objects with e.g. very long string keys.

It makes no difference. We are only comparing the types, not the values themselves.

How does it handle circular references?

It does not. Circular reference cannot exist in a type declaration.

It does not matter if circular reference exists in the subject object.

How do we ensure that the client code can import the pretty-print-object module? (the user might not have specified it in package.json)

You bundle it as a dependency of babel-plugin-typecheck and use require.resolve('pretty-print-object') to inject it into the runtime.

Here is an example of me doing this in another babel plugin, https://github.com/gajus/pragmatist/blob/a04832b9aa97887691bd1d782b28cec8de5c5524/src/babel-plugin-source-map-support/index.js#L34.

This approach assumes that babel-plugin-typecheck is present as a dependency whenever its code is being used, i.e. if user chooses to use type checking in production, then babel-plugin-typecheck would need to be a direct dependency as opposed to devDependency.

phpnode commented 8 years ago

Circular reference cannot exist in a type declaration.

They can, e.g. this is valid:

type Tree = {
  value: any;
  parent?: Tree;
  left?: Tree;
  right?: Tree;
};

You bundle it as a dependency of babel-plugin-typecheck and use require.resolve('pretty-print-object') to inject it into the runtime.

If we're doing this I think that we'll need to inline the pretty print module into the script to avoid that kind of user pain, in the same way that babel generates a _createClass function when defining classes. This is pretty easy to do and the module is small / simple enough.

One more point - how do you handle e.g. StringLiteralTypeAnnotation? Do you quote it?

type state = "active" | "inactive";
// becomes:
var state = '"active"|"inactive"';
// and not:
var state = 'active|inactive';
gajus commented 8 years ago

For the record, the library has been renamed to prettyprint, https://github.com/gajus/prettyprint

gajus commented 8 years ago

One more point - how do you handle e.g. StringLiteralTypeAnnotation? Do you quote it?

It can be either.

gajus commented 8 years ago

If we're doing this I think that we'll need to inline the pretty print module into the script to avoid that kind of user pain, in the same way that babel generates a _createClass function when defining classes. This is pretty easy to do and the module is small / simple enough.

Wouldn't you need to inline it in every file then?

phpnode commented 8 years ago

Wouldn't you need to inline it in every file then?

Yes, but this is acceptable, I added this for the current "inspector" in #94

gajus commented 8 years ago

You are right in the sense that there isn't that much of the code.

Anyway, I am happy for you to proceed with the inline approach.

How can I help?

gajus commented 8 years ago

@phpnode have you had time to look into this?

phpnode commented 8 years ago

@gajus I just pushed a couple more commits to #94 which gives error messages like this:

Function "demo" return value violates contract.

Expected:
{ [key: string]: string | number
}

Got:
{ string: string;
  number: number;
  foo: boolean;
}
phpnode commented 8 years ago

Fixed in 3.6.0 but can probably be further improved.