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

Better error messages #46

Open
davecoates opened this issue Apr 22, 2017 · 3 comments
Open

Better error messages #46

davecoates opened this issue Apr 22, 2017 · 3 comments

Comments

@davecoates
Copy link
Member

After using typed-immutable on multiple projects now the biggest problem that comes up is tracking down why a type error occurs - particularly when restoring a large nested structure from a raw JS object.

As an example:

const Address = Record({
  line1: String,
  city: String,
  country: String,
});

const Person = Record({
  id: Number,
  name: String,
  email: String,
  age: Number,
  address: Address,
});

const People = new Map(Number, Person);

new People([[1, { 
  id: 1,
  name: 'Bob',
  email: 'bob@example.com',
  age: 100,
  address: {
    line1: 5,
  }
}]]);

currently gives an error

TypeError: Invalid value: Invalid value for "address" field:
 Invalid value for "line1" field:
 "undefined" is not a string

This is ok when you have the context and know what structures it's referring to be in many cases you don't or the fields are generic enough that it's hard to identify where it comes from. It's also very useful to know the data that failed.

I've been looking at improving them, eg. the above is now:

    TypeError: Entry in Map(Number, Person) failed to satisfy value type:

    Key:
    1

    Value:
    {
      "id": 1,
      "name": "Bob",
      "email": "bob@example.com",
      "age": 100,
      "address": {
        "line1": 5
      }
    }

    Failed to create instance of Person

      Value for field 'address' must satisfy type 'Address'

    Failed to create instance of Address

      Value for field 'line1' must satisfy type 'String'

    Invalid value: "5" is not a string

My only concern with adding this is that it will be a bit slower to generate this level of detail (eg. doing a JSON.stringify on value). In my use cases so far it wouldn't matter as I never catch the TypeError's - if they occur it's a bug that we fix.

Does anyone have any thoughts about this? Should it be opt in? Am I overthinking it? I also haven't measured anything yet - I think it may only become a problem if you were generating a lot of these (eg. iterating a large list attempting to instantiating record's and handling any errors).

@lukesneeringer @stutrek @udnisap

@stutrek
Copy link
Member

stutrek commented Apr 23, 2017

I am always in favor of more descriptive error messages. If the speed of generating errors is holding back your app, something went wrong long before you chose to use typed-immutable. I do think the JSON should be opt in, since you might be logging a gigantic array of gigantic objects and the rest of your text should help find the error just fine.

I don't know if it's the line breaks or if it's longer than I was expecting, but can the message be made more concise? Even if not, I'm in favor.

Do you have a PR for it?

@davecoates
Copy link
Member Author

No PR yet, just basic implementation for Maps / Records, need to extend it to other types.

It has occurred to me that Union's rely on generating TypeErrors so will need to consider that in final implementation.

In terms of size of message it's currently giving you the full trace of each type that failed to instantiate and what the expected type was. Perhaps would be better to just give the path:

TypeError: Entry in Map(Number, Person) failed to satisfy value type for path 'address -> line1':

Invalid value: "5" is not a string

    Key:
    1

    Value:
    {
      "id": 1,
      "name": "Bob",
      "email": "bob@example.com",
      "age": 100,
      "address": {
        "line1": 5
      }
    }

The stuff below Invalid value: "5" is not a string could be made optional somehow as that's potentially problematic for large data structures. Not sure best way to do that though - global opt in of some sort?

import { setVerboseDebugging } from 'typed-immutable';

setVerboseDebugging(true);

@lukesneeringer
Copy link
Contributor

I would really like this change also.

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

3 participants