fix: Dynamically import webpack in webpack plugin #171
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files
☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
![codecov[bot]](https://avatars.githubusercontent.com/in/254?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diff is well-structured and mitigates problems related to importing the top level webpack. However, this change assumes that the webpack package is always available in the same environment where this code runs. If it's not, createRequire could throw an error.
| @@ -1,5 +1,6 @@ | |||
| import { type BundleAnalysisUploadPlugin } from "@codecov/bundler-plugin-core"; | |||
| import * as webpack from "webpack"; | |||
| import { createRequire } from "node:module"; | |||
| import type * as TWebpack from "webpack"; | |||
There was a problem hiding this comment.
The import type declaration could be changed to a named import to make it more explicit. That way, instead of import type * as TWebpack from 'webpack';, it could be import type { Compiler } from 'webpack';.
| @@ -25,6 +26,10 @@ | |||
| await output.write(); | |||
| }, | |||
| webpack(compiler) { | |||
There was a problem hiding this comment.
You are using dynamic imports to load the webpack module at runtime. While this does provide flexibility, it can make the code harder to analyze and may have performance implications compared to static import due to the overhead of dynamic calls.
| @@ -25,6 +26,10 @@ | |||
| await output.write(); | |||
| }, | |||
| webpack(compiler) { | |||
| const webpack = createRequire(import.meta.url)( | |||
| "webpack", | |||
There was a problem hiding this comment.
The casting as typeof TWebpack assumes that the import will always be valid. If webpack is not available or the import is invalid, it will throw an error. Consider adding error handling or checks to ensure webpack is available before importing.
Bundle ReportBundle size has no change ✅
|
Bundle ReportChanges will decrease total bundle size by 220 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
| @@ -25,6 +26,10 @@ export const webpackBundleAnalysisPlugin: BundleAnalysisUploadPlugin = ({ | |||
| await output.write(); | |||
| }, | |||
| webpack(compiler) { | |||
| const webpack = createRequire(import.meta.url)( | |||
There was a problem hiding this comment.
I generally prefer if this statement is split up to make it easier to read.
const generatedRequire = createRequire(import.meta.url);
const webpack = generatedRequire('webpack') as typeof TWebpack;
could you also sanity check that this transpiles properly for the cjs build?
There was a problem hiding this comment.
Here's what's in the .cjs file:
const generatedRequire = node_module.createRequire(
typeof document === "undefined"
? require("u" + "rl").pathToFileURL(__filename).href
: (_documentCurrentScript && _documentCurrentScript.src) ||
new URL("index.cjs", document.baseURI).href,
);
const webpack = generatedRequire("webpack");And what's in the .mjs:
const generatedRequire = createRequire(import.meta.url);
const webpack = generatedRequire("webpack");There was a problem hiding this comment.
Unminified*
There was a problem hiding this comment.
I believe that looks right, since the cjs is using require directly
| @@ -1,5 +1,6 @@ | |||
| import { type BundleAnalysisUploadPlugin } from "@codecov/bundler-plugin-core"; | |||
There was a problem hiding this comment.
You have removed a static import of the webpack module and replaced it with a dynamic import later in the code. This change isn't inherently problematic, but it does make the dependency on webpack less clear read at first glance. It's generally best to clearly state all dependencies via static imports at the top of your code.
| @@ -1,5 +1,6 @@ | |||
| import { type BundleAnalysisUploadPlugin } from "@codecov/bundler-plugin-core"; | |||
| import * as webpack from "webpack"; | |||
| import { createRequire } from "node:module"; | |||
There was a problem hiding this comment.
You have introduced the node:module package, currently it's an experimental feature in Node.js. Using experimental features can introduce instability to your codebase and for dependency software like codecov's plugins, it might be best to stick with stable features to ensure widest compatibility.
There was a problem hiding this comment.
that is not true codecov bot, you're like 6 node versions late 😭
There was a problem hiding this comment.
this bot has been replaced ... i should really remove it
nicholas-codecov
deleted the
fix-dynamically-import-webpack-in-webpack-plugin
branch
Description
This PR updates the webpack plugin, to dynamically import webpack so that we shouldn't have any issues with conflicting webpack issues with NextJS