fix(cdk/table): show a real error message instead of a crash when a table column is missing - #33698
fix(cdk/table): show a real error message instead of a crash when a table column is missing#33698arturovt wants to merge 1 commit into
Conversation
…able column is missing Before: if a <cdk-table> row used a column that wasn't defined, the app would crash in production with a confusing error like "Cannot read properties of undefined (reading 'headerCell')" — no mention of which column was the problem. This message only showed up in dev mode; in production you just got a raw, useless crash. Now: the table always throws a clear error like `Could not find column with id "column_a".`, in both dev and production, so it's obvious what went wrong and which column to fix. Added a test that forces production mode and checks the table gives the helpful error instead of crashing.
| private _addStickyColumnStyles(rows: HTMLElement[], rowDef: BaseRowDef) { | ||
| const columnDefs = Array.from(rowDef?.columns || []).map(columnName => { | ||
| const columnDef = this._columnDefsByName.get(columnName); | ||
| if (!columnDef && (typeof ngDevMode === 'undefined' || ngDevMode)) { |
There was a problem hiding this comment.
The intent here was that the columns likely aren't changing between dev and production so we don't need to do this assertion in production.
There was a problem hiding this comment.
yeah totally get that, and agree we don't want an assertion running on
every render just for something that "shouldn't" happen.
but fwiw this literally hit us in prod and we never once saw it in dev,
so the columns-dont-change-between-envs assumption kinda broke down for
us here lol. and when it does happen the error you get is just "cannot
read properties of undefined (reading headerCell)" with a stack that's
100% internal cdk frames - no column name, nothing. took us a while to
even figure out which table/column was the problem.
what if instead of bringing back the assertion we just wrap the actual
dereference in a try/catch and only build the nicer message if it
throws? like:
try {
return column.headerCell.template;
} catch (error) {
throw new error with some code (columnId);
}afaik try/catch is basically free in v8 as long as it doesn't actually
throw, so this shouldn't add the per-render cost you're worried about -
it only does anything in the case that was already about to blow up
anyway. just makes the blow up say something useful instead of a bare
TypeError.
happy to gate it behind prod-only too if you want dev to keep the exact
message it has now, just seemed silly to keep the error a total mystery
for anyone hitting in apps.
There was a problem hiding this comment.
this is how it looks in prod which isn't very friendly:
2 participants
Before: if a row used a column that wasn't defined, the app would crash in production with a confusing error like "Cannot read properties of undefined (reading 'headerCell')" — no mention of which column was the problem. This message only showed up in dev mode; in production you just got a raw, useless crash.
Now: the table always throws a clear error like
Could not find column with id "column_a"., in both dev and production, so it's obvious what went wrong and which column to fix.Added a test that forces production mode and checks the table gives the helpful error instead of crashing.