-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(errors-in-console): add ignoredPatterns option #9480
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely will need this for doing assertions in lhci
/** @return {AuditOptions} */ | ||
static defaultOptions() { | ||
return { | ||
ignoredPatterns: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forget what we prefer type-wise.. should this be null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's null
for all of our constants that end up in config.settings
because we want to distinguish between the default and unset state, but here I don't think we have a preference. willing to accept whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to do explicit undefined
over optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really other than a more legible version of the typedef for documentation, will remove
length: 8, | ||
}, | ||
items: [ | ||
{description: /Application Cache Error/}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these just got explicit so we'll need to resolve the conflict
@patrickhulce feel free to take a pass on the expectations whenever |
Oh I missed this ever got approval sorry! Will do 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it's going to be incredibly obscure, even for people specifically looking for this kind of option. Worth documenting in the web.dev explainer?
/** @return {AuditOptions} */ | ||
static defaultOptions() { | ||
return { | ||
ignoredPatterns: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to do explicit undefined
over optional?
} | ||
|
||
return true; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are we going to do with poorly formed ignoredPatterns
? The error messages aren't going to be very comprehensible (e.g. not an Array) and the silent failure when there isn't an exception (e.g. misspelled or not a string or regex) isn't going to be noticeable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeError: ignoredPatterns is not iterable
seems like a quite comprehensible error to fail with, IMO, especially given the sophistication necessary to set audit options at all.
I'm not sure how we could ever possibly know that a user has misspelled the error they want to ignore, so you've lost me on that one. users gotta learn to crawl on their own sometime 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeError: ignoredPatterns is not iterable
seems like a quite comprehensible error to fail with, IMO, especially given the sophistication necessary to set audit options at all.
yeah, that does read well
I'm not sure how we could ever possibly know that a user has misspelled the error they want to ignore, so you've lost me on that one. users gotta learn to crawl on their own sometime 😆
options: {ignoredPatternzzz: [/s.mple/i, 'really']}
and nothing happens. How do I figure out why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooooh misspell the option name, that makes much more sense. 👍
Honestly, I strongly dislike the pattern we've been starting to apply that unknown properties are fatal errors which breaks backwards/forwards compatibility. Would a warning of unrecognized property names address your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I strongly dislike the pattern we've been starting to apply that unknown properties are fatal errors which breaks backwards/forwards compatibility. Would a warning of unrecognized property names address your concern?
yeah, sounds great 👍
|
||
/** | ||
* @template {{description: string | undefined}} T | ||
* @param {Array<T>} items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only ever called with /** @type {Array<{source: string, description: string|undefined, url: string|undefined}>} */
, which is already declared once. Should just be a typedef instead of parametric? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why though? this method operates on things that have an optional description, no matter what they look like, and that's exactly how the types are expressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why though? this method operates on things that have an optional description, no matter what they look like, and that's exactly how the types are expressed
this is a nit, so can leave it, but it's only generic in the sense that all monomorphic functions are generic (over one type). Adding a parameter implies complexity that isn't there (it's only ever called with one type of thing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, and if typescript had another method of expressing monomorphic functions that operate on a subset of the type while preserving the output type I would happily switch to that method :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
method of expressing monomorphic functions that operate on a subset of the type while preserving the output type
I mean...
/**
* @param {Array<ConcreteType>} items
* @param {AuditOptions} options
* @return {Array<ConcreteType>}
*/
static filterAccordingToOptions(items, options) {
// ...
}
operates on a subset of the type just fine :P
(I blame everyone's emojis for the continuation of this conversation 👻📠)
Summary
Adds the
ignoredPatterns
audit option toerrors-in-console
which takes an array ofRegExp
orstring
s to filter the resulting text descriptions by.Related Issues/PRs
closes #9074