elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.84k stars 582 forks source link

Better DX: Show only app frames by default #1186

Closed b1az closed 9 months ago

b1az commented 9 months ago

I don't have data to back it up, but I'd venture a guess that, more often than not, the cause of an error lies somewhere in developer's own code. Thus, rather than being shown first Ecto's own code in lib/ecto/changeset.ex (as an example), it'd be a better developer experience to be first shown one's own code.

josevalim commented 9 months ago

I believe we had this as default at some point and, because the Erlang VM has tail call optimization, skipping the frames would often point nowhere helpful or somewhere too far away. So I'd prefer to keep things as is, thanks!

b1az commented 9 months ago

@josevalim, thanks for the feedback. Do I understand correctly that, since the proposed change is merely presentational, the 'skipped frames' issue you described is still present, just not immediately visible (until the "Show only app frames" gets checked)? In other words, when this issue occurs, isn't it merely a toggleOnclick() away from showing up? Regardless, if this issue occurs only rarely, it still might be prefered to have a better developer experience the majority of time.

But I perfectly understand that I might be missing some finer points, and that it's better to keep it as is. Out of curiosity, is this problem of 'skipped frames' at least not already known to be unsolveable? Might be something worth tackling, given how better, in my opinion at least, showing only app-frames by default is. I'd be grateful if you have any pointers at hand, in form of relevant commits or anything else. 🙏

josevalim commented 9 months ago

The "issue" I mentioned is always present. Tail call optimization is always done by the Erlang VM and it is an essential part of algorithms and modelling. So when both tail call optimization and show only app frames are enabled, sometimes it may end-up discarding too much information and ultimately confusing.

b1az commented 9 months ago

Understood about TCO. Was just hoping we could show a fallback, f.e. when none of the @frames have their .context set to :app. AFAIU, whatever info frames/3 managed to gather is there, regardless of what the state of the HTML view into it is (i.e. is the CSS -show-all class set or not). Thus, we could fall back to showing all app frames when not a single :app context was found (and also hide the unusable checkbox). @josevalim, would such an approach be acceptable? (You might have guessed by now I'd really like to have this default be changed. 😄)

I think this would cover the case when the stacktrace is mostly stripped away and no :app-specific frames are found (and whatever remains likely won't be useful anyway). FWIW, I can't recall when the currently default-shown/non-app stacktraces (f.e. from Enum, Ecto, etc.) were ever useful. So, anything greater than 0% is a win in my book. :-)