ref: Remove references to Webpack 4 #43
Conversation
Codecov ReportAttention:
Additional details and impacted files
☔ View full report in Codecov by Sentry. |
![codecov[bot]](https://avatars.githubusercontent.com/in/254?s=60&v=4){kind=small}
There was a problem hiding this comment.
The presented Git diff includes changes in several files, mainly intended to remove dependencies and references to webpack 4 and utilize webpack as a peer dependency instead. Here I have reviewed the changes, and all look fine, but there are some observations and potential improvements.
| import { findFilenameFormat } from "./findFileFormat"; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore webpack is a peer dep |
There was a problem hiding this comment.
Ignoring the type check for webpack import might lead to issues. Ensure types are consistent and correctly recognized for the webpack import.
There was a problem hiding this comment.
m: You can add webpack as a dev dep to get around this
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Show resolved
Hide resolved
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Show resolved
Hide resolved
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Show resolved
Hide resolved
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Show resolved
Hide resolved
| import { findFilenameFormat } from "./findFileFormat"; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore webpack is a peer dep |
There was a problem hiding this comment.
m: You can add webpack as a dev dep to get around this
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Outdated
Show resolved
Hide resolved
packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
Outdated
Show resolved
Hide resolved
|
Why are we aliasing the webpack dep? Keeping it as We can peer dep to |
I may just be wanting to be ahead of the curve for adding in V4 support down the road. Adding it as a peer dep broke the build quite badly, it's reading swc types for some reason and causing syntax errors to pop up. |
| @@ -42,8 +42,7 @@ | |||
| "test:unit:ci": "jest --coverage" | |||
| }, | |||
| "dependencies": { | |||
| "@codecov/bundler-plugin-core": "workspace:^", | |||
| "webpack": "5.89.0" | |||
| "@codecov/bundler-plugin-core": "workspace:^" | |||
| }, | |||
There was a problem hiding this comment.
Just to confirm, moving '@codecov/bundler-plugin-core' inside dependencies but moving 'webpack' out of it, into devDependencies could impact runtime if webpack is a runtime dependency. Please ensure that webpack is only needed in development and not in production.
| @@ -55,10 +54,11 @@ | |||
| "msw": "^2.0.9", | |||
| "ts-node": "^10.9.1", | |||
| "typescript": "^5.3.2", | |||
| "unbuild": "^2.0.0" | |||
There was a problem hiding this comment.
It's good that you've moved 'webpack' into 'devDependencies', but please make sure that every development environment installs dev dependencies. This may not be the case in certain CI/CD pipelines.
Description
This PR removes references to Webpack 4 in the codebase, and moves to using Webpack as a peer dep rather then a direct dependency.
Closes codecov/engineering-team#953
Notable Changes
webpack