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

Improved error messages #487

Open
evelant opened this issue Nov 6, 2022 · 5 comments
Open

Improved error messages #487

evelant opened this issue Nov 6, 2022 · 5 comments
Labels
🍗 enhancement New feature or request

Comments

@evelant
Copy link

evelant commented Nov 6, 2022

Would it be easy to add more data to runtime error messages? I'm finding with a large state tree that the current error messages give very little information to help locate the problem.

For example when data doesn't match in fromSnapshot you get an error like [Error: TypeCheckError: [/statusEffectDocId] Expected: string] which doesn't contain enough information to effectively figure out what bit of data is wrong.

I think errors on fields would be much more useful if they also contained the following:

  1. The invalid value. Currently it's impossible to tell why a type check failed because the invalid value isn't included.
  2. The full path to the invalid value from the root of whatever is being instantiated. Most of the time these errors come up when instantiating large nested models from snapshots. For example the above /statusEffectDocId would be much more useful if it were something like (fromSnapshot: PlayerModel) /items/equippedLeftHand/activeStatusEffects/statusEffectDocId. Without the full path it's not possible to know exactly where the invalid value is.
  3. The full path up to the root store if assigning a value to a prop in an existing store fails

Poking around the code I'm having trouble figuring out if any of this information is easily available to include in errors. Thoughts @xaviergonz ? Would this potentially be easy? The current error messages give so little context info that it takes a lot of time and effort to track down what's going wrong even though mobx-keystone likely has all that information internally.

@evelant
Copy link
Author

evelant commented Nov 7, 2022

Another example:

throw failure(`unsupported snapshot - ${sn}`)

just unsupported snapshot: [object Object] gives no useful information to help debug why the snapshot is bad or where the snapshot is being instantiated

@xaviergonz xaviergonz added the 🍗 enhancement New feature or request label Nov 15, 2022
@xaviergonz
Copy link
Owner

The invalid value. Currently it's impossible to tell why a type check failed because the invalid value isn't included.

That just got added in 1.2.0

  1. The full path to the invalid value from the root of whatever is being instantiated. Most of the time these errors come up when instantiating large nested models from snapshots. For example the above /statusEffectDocId would be much more useful if it were something like (fromSnapshot: PlayerModel) /items/equippedLeftHand/activeStatusEffects/statusEffectDocId. Without the full path it's not possible to know exactly where the invalid value is.
  2. The full path up to the root store if assigning a value to a prop in an existing store fails

I think this should be already the case. Do you have a codesandbox that shows this problem?

throw failure(unsupported snapshot - ${sn})

In that particular case, if you get there it usually means whatever you passed to fromSnapshot was not even JSON-compatible (a map, a set, an instance of a date...)

@evelant
Copy link
Author

evelant commented Nov 16, 2022

@xaviergonz here's an example https://codesandbox.io/s/xenodochial-cookies-21lwg3?file=/src/model.ts

import { fromSnapshot, Model, model, tProp, types as t } from "mobx-keystone";

@model("TestModel3")
class TestModel3 extends Model({
  test3_1: tProp(t.string)
}) {}

@model("TestModel2")
class TestModel2 extends Model({
  test2_1: tProp(t.string),
  test2_2: tProp(t.model(TestModel3))
}) {}

@model("TestModel1")
class TestModel1 extends Model({
  test1_1: tProp(t.string),
  test1_2: tProp(t.model(TestModel2))
}) {}

const snapshot: { [key: string]: any } = {
  $modelType: "TestModel1",
  test1_1: "asdf",
  test1_2: {
    $modelType: "TestModel2",
    test2_1: "fdas",
    test2_2: { $modelType: "TestModel3", test3_1: 123 }
  }
};
export const test = fromSnapshot(TestModel1, snapshot);

That will throw the error Error: TypeCheckError: [/test3_1] Expected: string. It only contains the path within the enclosing model, not the path all the way back to the type being instantiated. This is what can make it very difficult to know where something is wrong.

If I have a large snapshot with many different layers and types of nested models it can be impossible or very difficult to know where the problem came from. The above example is basic but some places in my app load snapshots with hundreds of keys, some nested inside maps or arrays. Without the full path to the root type being instantiated it's difficult to locate the bad data given just the prop name.

It would also be super helpful if mobx-keystone printed the modelIds (if it has it) for a failed typecheck on any property of a model. That would make it much easier to find the specific piece of bad data. Knowing that one field is bad doesn't help much if it's one field on one item in an array of 400 snaphots. A modelId would the error message would greatly help with finding exactly where the data is wrong.

In the above example, I'd love to see an error message like this if possible: Error: TypecheckError instantiating "TestModel1" at root (type: TestModel1 , modelId: 123) -> test2_2 (type: TestModel2, modelId: 234) -> test_3_1 (type: TestModel3, modelId: 456). Property "test_3_1" on model "TestModel3" with id 456 expected "string", got 123 (number)

An error like that example gives all the information necessary to know exactly why the type didn't match and exactly where the piece of data is based on the snapshot you're constructing from.

Does that help clarify the issue?

@evelant
Copy link
Author

evelant commented Jan 3, 2023

@xaviergonz here's an actual example from my app

Error [[snapshot '{"length":3}' does not match the following type: Array<string> | undefined | null]] Error adding activitiesLists id "sXuV8ZqMxTXvzb6EyXFH3AqaYgr1_01GFS9TYTFXQ4V9B8P3CC3GXRG"

The model in question, ActivitiesList, has over 100 properties and many nested sub-models. This error doesn't give me much of a clue about where exactly the bad data is located. All I know from this error is that some optional string array property is getting invalid input.

It would be much more helpful if mobx-keystone would identify the name of the invalid field along with the full path to the root of whatever is being constructed (in the case of sub-models inside of sub-models etc)

@evelant
Copy link
Author

evelant commented Mar 22, 2023

I'm still struggling with this fairly frequently. Here's another example Error: snapshot '"2"' does not match the following type: number | undefined | null. I have no idea what property on what model has the invalid value of "2". It could be any of hundreds of properties inside nested models of the one I'm constructing from a snapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants