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

Instrumenting panic messages #292

Open
soypat opened this issue Jan 14, 2022 · 8 comments
Open

Instrumenting panic messages #292

soypat opened this issue Jan 14, 2022 · 8 comments

Comments

@soypat
Copy link
Collaborator

soypat commented Jan 14, 2022

I often get panic messages with vecty and it is quite frustrating to debug since the whole stack is on vecty's side. It would be very nice if instead of

panic: vecty: next child render must not equal previous child render (did the child Render illegally return a stored render variable?)

We could get rich data on which node failed. I believe this could be done via reflect. I'll start asking around to see how other packages do this rich data printing, meanwhile I can add that these errors are strong deterrents to newcomers who have no idea what they mean.

@slimsag
Copy link
Member

slimsag commented Jan 14, 2022

I think this is probably more a documentation issue. If you're getting such panics, it means you're violating a core principle of Vecty. That should be happening very rarely. This isn't something you should be running into frequently.

In this case: it means that one of your component's Render methods did not return a new component instance (&Foobar{}) but instead returned some component that you stored somewhere else, in a field, variable, or otherwise just generally created outside of the Render method.

The whole idea / point of Vecty is that your Render method should construct an entirely new set of components / HTML each time it is called, and Vecty's diffing algorithm will ensure the browser DOM reflects it properly and efficiently. If you aren't doing that, you are introducing external state which is forbidden (because then you lose the benefits of using Vecty in the first place.)

Hope that helps, but yeah documentation is missing because of the current status of the project overall.

As an aside, we cannot use fmt.Sprintf which is traditionally used for making such panic messages nicer, because fmt is too big of a dependency to pull in.

@VinceJnz
Copy link

VinceJnz commented Jan 14, 2022

This is an area I have been struggling with and have spent a lot of time trying to understand. I had reached the point where I was wondering if I should just stop using vecty and not use a framework.

@soypat
Copy link
Collaborator Author

soypat commented Jan 14, 2022

I'm well aware of how I should be programming, and I feel I've done just that in this project https://github.com/soypat/mdc/blob/main/examples/appbar/appbarexample.go , but I'm still getting a panic on body rerender. I feel it must be something subtle and easy to miss.

In any case, you don't need fmt package- maybe if you could add to the panic messsge the html tag of the offending element and some metadata to aid with debugging, that would be a start.

@soypat
Copy link
Collaborator Author

soypat commented Jan 14, 2022

So I found the mistake! Although I was intending to create new components on every render call I was using stored components because I had not marked them with vecty:"prop"! Maybe a hint could be added to the panic message to check for this pitfall. I can make a PR later today

@slimsag
Copy link
Member

slimsag commented Jan 14, 2022

Sounds great to me! I think maybe my original message must have sounded dismissive? That was not my intent, I was just trying to explain why I thought docs are likely the biggest source of this confusion (but not in your case it sounds like)

I definitely believe this is a real problem, and if we can use 'reflect' on its own to derive some type name info into these panic messages or similar I'm happy to accept such a PR!

@slimsag
Copy link
Member

slimsag commented Jan 14, 2022

Another thought: there was a linter planned that would catch such problems (passing a component to another as a field without a prop tag)

@soypat
Copy link
Collaborator Author

soypat commented Jan 15, 2022

@slimsag awesome, I'll think about how to go about it in a lean fashion. As for now I've gotten the hang of vecty (i think?) and have been plowing through bindings for my own implementation of Material Design Components over at soypat/mdc. I was wondering if you'd accept an issue on adding it to vecty/* || hexops/* once it's sorta fleshed out.

@slimsag
Copy link
Member

slimsag commented Jan 15, 2022

I was wondering if you'd accept an issue on adding it to vecty/* || hexops/* once it's sorta fleshed out.

See discussion in #188 - in general, I don't want to make anything here "official" without giving it a lot of thought. There are a lot of tradeoffs. I probably won't have time to get around to this anytime soon, though.

That said, I'm very happy to link to other projects/libraries like these so that they're easy to discover! We do that in two places today:

We can make these "stick out" more (for example, centralizing all such links into the projects-using-vecty.md file and then making it even more obvious from the README.md that file exists and contains Material Design libraries, etc.

How's that sound? Are you interested in sending PRs for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants