webpack: handle a couple more cfg.entry formats #197
Conversation
src/plugins/webpack/index.ts
Outdated
| else if (typeof entry === "function") entries.push((entry as () => string)()); | ||
| else if (entry && typeof entry === "object" && "filename" in entry) entries.push(entry["filename"] as string); | ||
| else { | ||
| // TODO: warn about unrecognized single entry in object |
There was a problem hiding this comment.
It's not Knip's responsibility to validate/lint the configuration of external tooling. The plugins just try to find dependencies.
There was a problem hiding this comment.
I wouldn't say it's about validation or linting, but instead making it easier to detect when Webpack changes/extends its configuration schema and the plugin doesn't recognize it. Rather than crash as happened in my case :P
I left this note in case knip has a preferred way of surfacing such warnings. But I can remove the comments if you like.
There was a problem hiding this comment.
There's no handling of such config issues (I'd leave that to e.g. Webpack in this case), other than handling valid configuration.
There was a problem hiding this comment.
Right -- I'm not sure you've understood me. The scenario here is that some configuration is perfectly valid according to Webpack, but hasn't been handled in Knip for some reason. So a warning would be useful to make it clear that Knip needs to handle a new case.
But it sounds like you don't want it, so I'll remove these branches.
There was a problem hiding this comment.
Thanks, what you describe is the ongoing process of what is Knip is all about, to keep iterating and improving based on fixes like yours. Unless you already have a specific case in mind that you know should be handled, the comment does not add much for me.
There was a problem hiding this comment.
Oh well the suggestion was to turn the comment into a console.warn, or some other suitable way of surfacing a warning.
That way the next person who runs into the same scenario won't have to decipher an opaque error. (My error was an undefined string deep inside some stacktrace.)
There was a problem hiding this comment.
But no worries either way, my use-case works after this fix so I'm happy.
There was a problem hiding this comment.
Exception handling in general is something that should be improved indeed!
|
🚀 This pull request is included in v2.19.3. See Release 2.19.3 for release notes. |
|
Thanks again Tom! 🙏 |
|
No problem, thanks for the fast review! |
This fixes a crash I was getting in my project based on Razzle.js, because the Webpack plugin code wasn't handling all of the cases that can happen in the configuration's
entryfield. (Specifically, when the entry is an object and its keys are of typestring[].)This is also a little more defensively written and (IMO) easier to understand.
Based on the TS types here:
https://github.com/webpack/webpack/blob/1f99ad6367f2b8a6ef17cce0e058f7a67fb7db18/types.d.ts#L2396-L2400