Skip to content
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

Merged
merged 4 commits into from
Jun 19, 2023
Merged

Conversation

goloveychuk
Copy link
Contributor

@goloveychuk goloveychuk commented Dec 19, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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

@goloveychuk
Copy link
Contributor Author

reapprove please

@shellscape
Copy link
Owner

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);
Copy link
Owner

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

Copy link
Contributor Author

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"

Copy link
Contributor Author

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

Copy link
Owner

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.

Copy link
Contributor Author

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;

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@shellscape
Copy link
Owner

made more updates to upstream/master. webpack needed an upgrade to support node v18 in the tests.

@goloveychuk
Copy link
Contributor Author

rebased

@goloveychuk
Copy link
Contributor Author

I think there is no better fix, wdyt?

@wlblanchette
Copy link

Wanted to drop a comment here that this fix solves an instance of TypeError: The 'compilation' argument must be an instance of Compilation that results from a monorepo environment where the webpack configuration is defined in one package, with one instance of webpack and then imported into a second package where, for whatever reason, the second package its own instance of webpack.

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.

@shellscape shellscape changed the title Removes webpack import fix: only import webpack types Jun 19, 2023
@shellscape shellscape merged commit 64faade into shellscape:master Jun 19, 2023
5 checks passed
@shellscape
Copy link
Owner

Thanks for being patient on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants