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.

Sign up for GitHub

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

feat: Add in bundle name config option #26

Merged
merged 10 commits into from
Dec 19, 2023
Merged

feat: Add in bundle name config option #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4b1474e
add bundle name config option to bundler core
nicholas-codecov Dec 13, 2023
26098f0
update unit tests
nicholas-codecov Dec 13, 2023
ccd55ed
update integration tests
nicholas-codecov Dec 13, 2023
f37a830
make bundle name optional for future incase we expand our plugins
nicholas-codecov Dec 13, 2023
df6652b
early return so we don't attempt to upload
nicholas-codecov Dec 13, 2023
2da2a19
emit warning from the various bundler plugins
nicholas-codecov Dec 13, 2023
acba306
append bundler name option if present
nicholas-codecov Dec 14, 2023
4f9e05b
append output format to bundle name
nicholas-codecov Dec 14, 2023
92ae7b4
update integration tests
nicholas-codecov Dec 14, 2023
3930adc
Merge branch 'main' into add-in-bundle-name-config-option
nicholas-codecov Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion integration-tests/fixtures/generate-bundle-stats/rollup-plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ describe("Generating rollup stats", () => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
resolve(),
commonjs(),
codecovRollupPlugin({ enableBundleAnalysis: true, dryRun: true }),
codecovRollupPlugin({
enableBundleAnalysis: true,
dryRun: true,
bundleName: "rollup-test",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundleName option is added to the plugin, but it is not clear what it means or how it is used within the plugin itself. It could be helpful to add comments explaining what it does.

}),
],
}).then((bundle) =>
bundle.write({
Expand Down Expand Up @@ -89,4 +93,8 @@ describe("Generating rollup stats", () => {
it("sets the correct bundler information", () => {
expect(stats.bundler).toStrictEqual(expectedStats.bundler);
});

it("sets the correct bundle name", () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test checks if the bundleName in the stats is correctly set to the one provided in the plugin options. However, it is not clear how this bundleName is used in the test or the functionality it provides.

expect(stats.bundleName).toStrictEqual("rollup-test-es");
});
});
10 changes: 9 additions & 1 deletion integration-tests/fixtures/generate-bundle-stats/vite-plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ describe("Generating vite stats", () => {
},
},
plugins: [
codecovVitePlugin({ enableBundleAnalysis: true, dryRun: true }),
codecovVitePlugin({
enableBundleAnalysis: true,
dryRun: true,
bundleName: "vite-test",
}),
],
});

Expand Down Expand Up @@ -88,4 +92,8 @@ describe("Generating vite stats", () => {
it("sets the correct bundler information", () => {
expect(stats.bundler).toStrictEqual(expectedStats.bundler);
});

it("sets the correct bundle name", () => {
expect(stats.bundleName).toStrictEqual("vite-test-cjs");
});
});
10 changes: 9 additions & 1 deletion integration-tests/fixtures/generate-bundle-stats/webpack-plugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ describe("Generating webpack stats", () => {
},
mode: "production",
plugins: [
codecovWebpackPlugin({ enableBundleAnalysis: true, dryRun: true }),
codecovWebpackPlugin({
enableBundleAnalysis: true,
dryRun: true,
bundleName: "webpack-test",
}),
],
},
(err) => {
Expand Down Expand Up @@ -95,4 +99,8 @@ describe("Generating webpack stats", () => {
it("sets the correct bundler information", () => {
expect(stats.bundler).toStrictEqual(expectedStats.bundler);
});

it("sets the correct bundle name", () => {
expect(stats.bundleName).toStrictEqual("webpack-test-array-push");
});
});
6 changes: 3 additions & 3 deletions ...ges/bundler-plugin-core/src/bundle-analysis/__tests__/bundleAnalysisPluginFactory.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { bundleAnalysisPluginFactory } from "../bundleAnalysisPluginFactory";
describe("bundleAnalysisPluginFactory", () => {
it("returns a build start function", () => {
const plugin = bundleAnalysisPluginFactory({
userOptions: {},
userOptions: { bundleName: "test" },
bundleAnalysisUploadPlugin: () => ({
version: "1",
name: "plugin-name",
Expand All @@ -16,7 +16,7 @@ describe("bundleAnalysisPluginFactory", () => {

it("returns a build end function", () => {
const plugin = bundleAnalysisPluginFactory({
userOptions: {},
userOptions: { bundleName: "test" },
bundleAnalysisUploadPlugin: () => ({
version: "1",
name: "plugin-name",
Expand All @@ -29,7 +29,7 @@ describe("bundleAnalysisPluginFactory", () => {

nicholas-codecov marked this conversation as resolved.
Show resolved Hide resolved
it("returns a write bundle function", () => {
const plugin = bundleAnalysisPluginFactory({
userOptions: {},
userOptions: { bundleName: "test" },
bundleAnalysisUploadPlugin: () => ({
version: "1",
name: "plugin-name",
Expand Down
5 changes: 4 additions & 1 deletion packages/bundler-plugin-core/src/bundle-analysis/bundleAnalysisPluginFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ export const bundleAnalysisPluginFactory = ({
userOptions,
bundleAnalysisUploadPlugin,
}: BundleAnalysisUploadPluginArgs): UnpluginOptions => {
// const dryRun = userOptions?.dryRun ?? false;
const output: Output = {
version: "1",
bundleName: userOptions.bundleName ?? "",
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundleName is added to the Output object here. It would be useful to know where this object is used and what effect the bundleName has on it.


const { pluginVersion, version, ...pluginOpts } = bundleAnalysisUploadPlugin({
Expand All @@ -47,6 +47,9 @@ export const bundleAnalysisPluginFactory = ({
// don't need to do anything here if dryRun is true
if (userOptions?.dryRun) return;

// don't need to do anything if the bundle name is not present or empty
nicholas-codecov marked this conversation as resolved.
Show resolved Hide resolved
if (!userOptions.bundleName || userOptions.bundleName === "") return;

const args: UploadOverrides = userOptions.uploaderOverrides ?? {};
const envs = process.env;
const inputs: ProviderUtilInputs = { envs, args };
Expand Down
4 changes: 3 additions & 1 deletion packages/bundler-plugin-core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ interface CodecovUnpluginFactoryOptions {
bundleAnalysisUploadPlugin: BundleAnalysisUploadPlugin;
}

export function codecovUnpluginFactory({
function codecovUnpluginFactory({
bundleAnalysisUploadPlugin,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codecovUnpluginFactory function is sightly modified, and it's not clear why. Perhaps it is related to the bundleName implementation but it needs clarification.

}: CodecovUnpluginFactoryOptions) {
return createUnplugin<Options, true>((userOptions, unpluginMetaContext) => {
Expand All @@ -48,6 +48,8 @@ export function codecovUnpluginFactory({
});
}

export { red, codecovUnpluginFactory };

export type {
BundleAnalysisUploadPlugin,
Asset,
Expand Down
10 changes: 10 additions & 0 deletions packages/bundler-plugin-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface Module {

export interface Output {
version?: string;
bundleName: string;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundleName field is added to the Output Interface. But, It seems to be mandatory in the Interface which might not be sensible as it's optional in Options Interface (and can be undefined).

bundler?: {
name: string;
version: string;
Expand Down Expand Up @@ -92,6 +93,15 @@ export interface Options {
* Defaults to `false`
*/
dryRun?: boolean;

/**
* The name for the bundle being built.
*
* Required for uploading bundle analysis information.
*
* Example: `rollup-package`
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bundleName field is added to the Options Interface, but it's marked as optional. It should be mandatory if it's required for uploading bundle analysis information.

bundleName?: string;
}
nicholas-codecov marked this conversation as resolved.
Show resolved Hide resolved

export type BundleAnalysisUploadPlugin = (
Expand Down
8 changes: 6 additions & 2 deletions ...ges/rollup-plugin/src/rollup-bundle-analysis/__tests__/rollupBundleAnalysisPlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ describe("rollupBundleAnalysisPlugin", () => {
describe("when called", () => {
it("returns a plugin object", () => {
const plugin = rollupBundleAnalysisPlugin({
output: {},
userOptions: {},
output: {
bundleName: "test",
},
userOptions: {
bundleName: "test",
},
});

expect(plugin.version).toEqual("1");
Expand Down
14 changes: 14 additions & 0 deletions packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
type Chunk,
type Module,
type BundleAnalysisUploadPlugin,
red,
} from "@codecov/bundler-plugin-core";

const PLUGIN_NAME = "codecov-rollup-bundle-analysis-plugin";
Expand All @@ -17,6 +18,19 @@
pluginVersion: "1.0.0",
rollup: {
generateBundle(this, options, bundle) {
// don't need to do anything if the bundle name is not present or empty
if (!userOptions.bundleName || userOptions.bundleName === "") {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If bundleName is not provided or is an empty string, red text stating 'Bundle name is not present or empty. Skipping upload.' is logged to the console. However, it's not sure if this behavior of skipping upload when bundleName is missing suits all use-cases.

red("Bundle name is not present or empty. Skipping upload.");
return;

Check warning on line 24 in packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts#L23-L24 

Added lines #L23 - L24 were not covered by tests
}

// append bundle output format to bundle name
output.bundleName = `${userOptions.bundleName}-${options.format}`;

Check warning on line 28 in packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts#L28 

Added line #L28 was not covered by tests

if (options.name && options.name !== "") {
output.bundleName = `${userOptions.bundleName}-${options.name}`;

Check warning on line 31 in packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/rollup-plugin/src/rollup-bundle-analysis/rollupBundleAnalysisPlugin.ts#L31 

Added line #L31 was not covered by tests
}

const customOptions = {
moduleOriginalSize: false,
...options,
Expand Down
8 changes: 6 additions & 2 deletions packages/vite-plugin/src/vite-bundle-analysis/__tests__/viteBundleAnalysisPlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ describe("viteBundleAnalysisPlugin", () => {
describe("when called", () => {
it("returns a plugin object", () => {
const plugin = viteBundleAnalysisPlugin({
output: {},
userOptions: {},
output: {
bundleName: "test",
},
userOptions: {
bundleName: "test",
},
});

expect(plugin.version).toEqual("1");
Expand Down
15 changes: 15 additions & 0 deletions packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
type Chunk,
type Module,
type BundleAnalysisUploadPlugin,
red,
} from "@codecov/bundler-plugin-core";

const PLUGIN_NAME = "codecov-vite-bundle-analysis-plugin";
Expand All @@ -17,6 +18,20 @@
pluginVersion: "1.0.0",
vite: {
generateBundle(this, options, bundle) {
// don't need to do anything if the bundle name is not present or empty
nicholas-codecov marked this conversation as resolved.
Show resolved Hide resolved
if (!userOptions.bundleName || userOptions.bundleName === "") {
red("Bundle name is not present or empty. Skipping upload.");
return;

Check warning on line 24 in packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts#L23-L24 

Added lines #L23 - L24 were not covered by tests
}

// append bundle output format to bundle name
output.bundleName = `${userOptions.bundleName}-${options.format}`;

Check warning on line 28 in packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts#L28 

Added line #L28 was not covered by tests

// add in bundle name if present
if (options.name && options.name !== "") {
output.bundleName = `${userOptions.bundleName}-${options.name}`;

Check warning on line 32 in packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/vite-plugin/src/vite-bundle-analysis/viteBundleAnalysisPlugin.ts#L32 

Added line #L32 was not covered by tests
}

const customOptions = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again here, the upload is skipped if bundleName is not present. There seems to be a pattern of operational changes depending on whether the bundleName exists.

moduleOriginalSize: false,
...options,
Expand Down
8 changes: 6 additions & 2 deletions .../webpack-plugin/src/webpack-bundle-analysis/__tests__/webpackBundleAnalysisPlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ describe("webpackBundleAnalysisPlugin", () => {
describe("when called", () => {
it("returns a plugin object", () => {
const plugin = webpackBundleAnalysisPlugin({
output: {},
userOptions: {},
output: {
bundleName: "test",
},
userOptions: {
bundleName: "test",
},
});

expect(plugin.version).toEqual("1");
Expand Down
19 changes: 18 additions & 1 deletion packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import { type BundleAnalysisUploadPlugin } from "@codecov/bundler-plugin-core";
import {
type BundleAnalysisUploadPlugin,
red,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Red is imported but unused. This needs to be reviewed and removed if it's not needed.

} from "@codecov/bundler-plugin-core";
import * as webpack4or5 from "webpack";

const PLUGIN_NAME = "codecov-webpack-bundle-analysis-plugin";
Expand All @@ -18,6 +21,20 @@
stage: webpack4or5.Compilation.PROCESS_ASSETS_STAGE_REPORT,
},
() => {
// don't need to do anything if the bundle name is not present or empty
if (!userOptions.bundleName || userOptions.bundleName === "") {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the context of what this bundleName is meant to contribute to the functionality of the plugins, the addition of it at so many points throughout these files lessens the readability and understanding.

red("Bundle name is not present or empty. Skipping upload.");
return;

Check warning on line 27 in packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts#L26-L27 

Added lines #L26 - L27 were not covered by tests
}

if (typeof compilation.outputOptions.chunkFormat === "string") {
output.bundleName = `${userOptions.bundleName}-${compilation.outputOptions.chunkFormat}`;

Check warning on line 31 in packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts#L31 

Added line #L31 was not covered by tests
}

if (compilation.name && compilation.name !== "") {
output.bundleName = `${userOptions.bundleName}-${compilation.name}`;

Check warning on line 35 in packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts

View check run for this annotation

Codecov / codecov/patch

packages/webpack-plugin/src/webpack-bundle-analysis/webpackBundleAnalysisPlugin.ts#L35 

Added line #L35 was not covered by tests
}

const compilationStats = compilation.getStats().toJson({
assets: true,
chunks: true,
Expand Down