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

Improve legibility of the shape mismatch error #93

Closed
gajus opened this issue Dec 11, 2015 · 16 comments
Closed

Improve legibility of the shape mismatch error #93

gajus opened this issue Dec 11, 2015 · 16 comments

Comments

@gajus
Copy link
Collaborator

gajus commented Dec 11, 2015

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
Copy link
Collaborator Author

gajus commented Dec 11, 2015

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

@gajus
Copy link
Collaborator Author

gajus commented Dec 11, 2015

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
Copy link
Member

phpnode commented Dec 11, 2015

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
Copy link
Collaborator Author

gajus commented Dec 13, 2015

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
Copy link
Collaborator Author

gajus commented Dec 13, 2015

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

@phpnode
Copy link
Member

phpnode commented Dec 14, 2015

@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
Copy link
Collaborator Author

gajus commented Dec 14, 2015

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
Copy link
Member

phpnode commented Dec 14, 2015

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
Copy link
Collaborator Author

gajus commented Dec 15, 2015

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

@gajus
Copy link
Collaborator Author

gajus commented Dec 15, 2015

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

It can be either.

@gajus
Copy link
Collaborator Author

gajus commented Dec 15, 2015

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
Copy link
Member

phpnode commented Dec 16, 2015

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
Copy link
Collaborator Author

gajus commented Dec 16, 2015

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
Copy link
Collaborator Author

gajus commented Jan 4, 2016

@phpnode have you had time to look into this?

@phpnode
Copy link
Member

phpnode commented Jan 4, 2016

@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 phpnode closed this as completed Jan 4, 2016
@phpnode phpnode reopened this Jan 4, 2016
@phpnode
Copy link
Member

phpnode commented Jan 4, 2016

Fixed in 3.6.0 but can probably be further improved.

@phpnode phpnode closed this as completed Jan 4, 2016
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

No branches or pull requests

2 participants