Added filterWarnings option, fixes #85 #108
Conversation
dist/index.d.ts
Outdated
| * A function to filter out warnings | ||
| * Defaults to a constant function that returns `true` | ||
| */ | ||
| filterWarnings?: Function; |
There was a problem hiding this comment.
I would change this to onwarn since that's what the option is called in the rollup/webpack/vite plugin
There was a problem hiding this comment.
The semantics are vastly different. I assume onwarn in the other plugins (I'm looking at Rollup) is executed for each warning in an (async?) event-type scenario (not in batches as it's done here) and that's why it's using a callback (so it can also do async stuff I assume?). Given the architecture here and that I have access to a static array of warnings I don't see a point forcing it into the same structure. Honestly how would that even work? I can't await all onwarn calls if handler is only called when you want to keep the warning and it's simply not called when you don't. How would I know it's done? Like I said, there's probably a reason but the API is awkward and not a pattern you often see. It's neither classic error-first callback nor is it compatible with sth. like async.filter where the iteratee needs to call the callback at all times, either with a truthy or falsy values.
I have the luxury here of doing a super basic Array#filter and I'm down for it.
index.ts
Outdated
| * A function to filter out warnings | ||
| * Defaults to a constant function that returns `true` | ||
| */ | ||
| filterWarnings?: Function; |
index.ts
Outdated
| @@ -237,7 +248,7 @@ export default function sveltePlugin(options?: esbuildSvelteOptions): Plugin { | |||
|
|
|||
| const result: OnLoadResult = { | |||
| contents, | |||
| warnings: warnings.map(convertMessage), | |||
| warnings: warnings.filter(options?.filterWarnings).map(convertMessage), | |||
There was a problem hiding this comment.
TS complains here / wants you to add the ? because control flow is reset inside functions. TS can't know for sure that there isn't something else that resets this options to undefined between you setting it to sth and this function getting called.
There was a problem hiding this comment.
I'm not questioning TSs decision here, but there is only a single assignment to options in that entire file. I know it will never be undefined 🤷
There was a problem hiding this comment.
You can do options!.filterWarnings in that case
|
This addition makes sense to me at first glance, thanks for the PR! I agree that I'll do a review and get this merged this weekend |
For context sveltejs/svelte#7240
I have no idea about TypeScript and why it thinks
optionscould be undefined given this:esbuild-svelte/index.ts
Lines 88 to 90 in 589870f
Anyway, if I disable the TypeScript checks then
npm run build && npm testsucceeds. Could you please clean this up (warnings.filter(options?.filterWarnings)doesn't look correct)? I need a way to filter out false positive warnings. I don't understand why Rollup uses a callback pattern. This PR solves my problem but might have some shortcoming? idk