-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix TypeScript types #1132
Fix TypeScript types #1132
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.
I really like what you did here!
Unfortunately sorting the options made it very hard to review this pull request.
Could you please add some changes like described above?
Sorry about the Options sorting thing. Should have dropped that in an independent commit. I'll push some changes! |
Just realized that the |
Hmm, I tested it locally. |
Ok, scratch that. Looks like |
Ok, so apparently I didn't need to redefine The other part is apparently you can remove the optionality aspect of a type but you can't remove its ability to be set to undefined (see here). So, this approach may not actually solve the use case I was trying to solve. As it's still internal it may not be that big of a deal but... |
Well right now the types were used to guarantee the quality and to reduce bugs.. |
Because they weren't accurate to begin with. If you're that worried about it feel free to revert this PR. Your TS support for downstream users is completely broken though. How are they supposed to guarantee quality and reduce bugs? |
They were very accurate for all internal flows and there was @types/html-webpack-plugin for downstream users. If an internal function relies on values which are not present it might cause bugs in edge case situations What was not accurate? |
I'm not going to debate this, sorry. I think #1108 + the explanations in this PR cover the reasons. So, looks like I spoke too soon. I poked at this some more and the problem is actually the wildcard You can see a simple example here on the TS playground. Comment out line 4 and you can see the errors change accordingly. |
I guess the cleanest way would be to write two interfaces or to start with the ProcessedOption and use Partial again |
Before we relied on the webpack source code - now we rely on @types/webpack |
No it doesn't. See https://github.com/jantimon/html-webpack-plugin/blob/master/index.js#L7-L8 |
But why did we have to add @types/webpack ? |
Thanks good hint :) I’ll try to refactor those changes on a new or tomorrow |
@wingrunr21 I fixed the remaining open points - would love to get your feedback: |
This resolves #1108
types
property topackage.json
(see Publishing docs)typings.d.ts
file to more accurately reflect both the internal and external API. Specifically, theOptions
interface should have every property marked optional (for external consumption) but most of those properties are assumed to be not null/undefined throughoutindex.js
. I thus introduced anInternalOptions
interface which matches this requirement.Hooks
typedef into thetypings.d.ts
file vs defining it inline.HtmlTagObject
inside theHtmlWebpackPlugin
namespace.I did not fix compatibility with
noImplicitAny
in this PR. As you are shipping an annotated JS file, I believe TS users downstream with bothnoImplicitAny
andallowJs
enabled will get a bunch of compiler warnings.cc @SimonSchick