More user friendly errors in layout.js #26448
Conversation
src/layout.js
Outdated
| userAssert(inputLayout !== undefined, 'Unknown layout: %s', layoutAttr); | ||
| userAssert( | ||
| inputLayout !== undefined, | ||
| 'Invalid "layout" value: %s', |
There was a problem hiding this comment.
Is it relatively trivial to include in this error message the name of the offending component?
There was a problem hiding this comment.
Added the element itself. 👍
| /** @const {string|null|undefined} */ | ||
| const inputHeight = | ||
| heightAttr && heightAttr != 'fluid' ? parseLength(heightAttr) : heightAttr; | ||
| userAssert(inputHeight !== undefined, 'Invalid height value: %s', heightAttr); |
There was a problem hiding this comment.
For my own learning, why do you prefer variable interpolation via %s instead of via backticks?
There was a problem hiding this comment.
We have a feature that extracts error strings at compile time and replaces them with an error code for a smaller binary size. It doesn't play nice with JS string templating unfortunately.
There was a problem hiding this comment.
Actually I'm poking around now and I'm not sure this is true and/or if the feature was launched. :)
|
Any other comments or can we merge? :) |
|
@choumx I admit I did make another suggestion about even friendlier error messages (is such a thing even possible?) Please see above.... |
|
@morsssss Are you talking about #26448 (comment)? I made a change for that already. |
|
@choumx I don't know if I can get a direct link to this particular comment - but it's the one above that suggests language like
and
instead of like
|
|
As is, devtools will render the element itself into console (e.g. supports highlight on hover). Try it out and let me know if you'd still prefer a prefix tag string. |
* master: (62 commits) 📦 Update dependency fetch-mock to v8.3.2 (ampproject#26491) Revert 'Move mutator implementations out to a standalone service' (ampproject#26479) Fix syntax error (ampproject#26481) Add pespective back. (ampproject#26486) More user friendly errors in layout.js (ampproject#26448) ✨ Start logging AMP URL on SwG Pages (ampproject#26480) Fix border around desktop amp-story-pages. (ampproject#26449) Fix Story tests. (ampproject#26464) ✨ Performance Measurement Chrome Extension (ampproject#26333) amp-consent restrict iframe fullScreen if no focus (ampproject#26461) Add performance benchmark task (ampproject#26026) ♻️ amp-script: emit warning if zero height and width. (ampproject#26444) ✨ Launch minimal-wrapper native CEv1 (ampproject#26360) ♻️ Lint: include externs (round 2) (ampproject#26446) amp-script: Create 'fill content' container for responsive/fluid (ampproject#26400) amp-consent remove cmp iframe focus (ampproject#26437) Disable macro-after-long-task in inabox. (ampproject#26459) Launch layoutbox-invalidate-on-scroll (ampproject#26430) Add amp-ad support for ByPlay (ampproject#25663) 🏗 Add specific RTV opt-in to experiments.html (ampproject#26434) ...
|
That highlight on hover will be excellent! (I don't get this yet in my v0.js) But I think that if it's just as easy to output friendlier, more human-readable errors, we should. I think that, otherwise, we're making ourselves less accessible than other programming frameworks. That said, this is not matter of broken functionality, so I'm not going to belabor the point :) |
|
just wanted to bump this one up again... |
|
Here's what the error looks like now: I think that's what you wanted right? |
|
That's really nice! |
Fixes #26398.