-
-
Notifications
You must be signed in to change notification settings - Fork 184
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: only import webpack types #297
Conversation
reapprove please |
Please rebase from upstream/master |
src/index.ts
Outdated
@@ -93,7 +91,7 @@ class WebpackManifestPlugin implements WebpackPluginInstance { | |||
const hook = !NormalModule.getCompilationHooks | |||
? compilation.hooks.normalModuleLoader | |||
: NormalModule.getCompilationHooks(compilation).loader; | |||
hook.tap(hookOptions, normalModuleLoader); | |||
hook.tap(hookOptions, normalModuleLoader as any); |
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.
The use of as any
here will be a blocker for this PR
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.
second try. Webpack types are incorrect. Afaik object type is not assignable to other "objects"
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.
so without casting it won't work
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.
look at the types before your changes (what's on master), and compare to what it is with your changes, adjust from there.
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.
before my change it has import
// @ts-ignore
import NormalModule from 'webpack/lib/NormalModule';
so NormalModule was any.
You suggest to replicate previous types, so I'll need to
const NormalModule = compiler.webpack.NormalModule as any;
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 think upcasting from object to more specific type inside normalmodule hook is better, more types are preserved/checked
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.
second try.
By this I meant that I've made second fix, removed as any.
04f92f0
to
14aedeb
Compare
made more updates to upstream/master. webpack needed an upgrade to support node v18 in the tests. |
rebased |
I think there is no better fix, wdyt? |
Wanted to drop a comment here that this fix solves an instance of In our case, it was a symptom of hoisting falling over for an unknown reason, despite the webpack versions being exactly the same. However, with a similar setup with slight differences in version, this would be reproducable. If it's helpful to have a repro of this situation, I would be happy to spin up an issue with the repro and link it to this PR. |
Thanks for being patient on this one. |
Will this be released on NPM as v5.0.1? Asking as we’ve had to patch this locally to make our builds work. |
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
So I've somehow messed with dependency tree (with monorepo+linking) and webpack loaded twise.
While you have setup webpack as peerDep and everything should be ok, I guess better to get webpack module provided instead of importing it. Also easier ts types