Skip to content

fix: Issue when hash length is set #182

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

Merged
merged 8 commits into from
Oct 18, 2024
Merged

fix: Issue when hash length is set #182

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
615b754
update normalizePath
nsdeschenes Oct 18, 2024
005813c
update tests
nsdeschenes Oct 18, 2024
a815490
add changeset
nsdeschenes Oct 18, 2024
dc0e665
small optimization to remove need for looping
nsdeschenes Oct 18, 2024
5e6f67c
also handle underscores
nsdeschenes Oct 18, 2024
275d0f0
fix some snapshots
nsdeschenes Oct 18, 2024
6f8d670
some simple tidy up
nsdeschenes Oct 18, 2024
d392379
remove the ai reviewer
nsdeschenes Oct 18, 2024
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
14 changes: 14 additions & 0 deletions .changeset/strong-readers-count.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
"@codecov/bundler-plugin-core": patch
"@codecov/bundle-analyzer": patch
"@codecov/nextjs-webpack-plugin": patch
"@codecov/nuxt-plugin": patch
"@codecov/remix-vite-plugin": patch
"@codecov/rollup-plugin": patch
"@codecov/solidstart-plugin": patch
"@codecov/sveltekit-plugin": patch
"@codecov/vite-plugin": patch
"@codecov/webpack-plugin": patch
---

Fix issue with normalizing paths with a custom hash length is set
2 changes: 1 addition & 1 deletion ...tion-tests/fixtures/generate-bundle-stats/rollup/__snapshots__/rollup-plugin.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,7 @@ exports[`Generating rollup stats 4 {"format":"amd","expected":"amd"} matches the
{
"gzipSize": 98808,
"name": "main-H2_1FSsQ.js",
"normalized": "main-H2_1FSsQ.js",
"normalized": "main-*.js",
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

Be careful with normalizing to a wildcard. Depending on how these paths are consumed, using '*' might have some unexpected side-effects.

"size": 577073,
},
],
Expand Down
2 changes: 1 addition & 1 deletion integration-tests/fixtures/generate-bundle-stats/vite/__snapshots__/vite-plugin.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,7 @@ exports[`Generating vite stats v5 {"format":"cjs","expected":"cjs"} matches the
{
"gzipSize": 26812,
"name": "assets/index-_9bu_Rar.js",
"normalized": "assets/index-_9bu_Rar.js",
"normalized": "assets/index-*.js",
"size": 72342,
},
],
Expand Down
8 changes: 4 additions & 4 deletions ...on-tests/fixtures/generate-bundle-stats/webpack/__snapshots__/webpack-plugin.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ exports[`Generating webpack stats 5 {"format":"commonjs","expected":"cjs"} match
}
`;

exports[`Generating webpack stats 5 source maps are enabled does not include any source maps 1`] = `
exports[`Generating webpack stats 5 {"format":"module","expected":"esm"} matches the snapshot 1`] = `
{
"assets": [
{
Expand All @@ -161,7 +161,7 @@ exports[`Generating webpack stats 5 source maps are enabled does not include any
},
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

The changes in snapshot test names here (lines 55-56 and 73-74) seem inverted. Double-check these name changes since the first test now has a snapshot name referring to ESM output instead of checking for non-inclusion of source maps, and vice versa in the second test.

],
"builtAt": Any<Number>,
"bundleName": StringNotContaining ".map",
"bundleName": StringContaining "test-webpack-v5-esm",
"bundler": {
"name": "webpack",
"version": "5.90.0",
Expand Down Expand Up @@ -225,7 +225,7 @@ exports[`Generating webpack stats 5 source maps are enabled does not include any
}
`;

exports[`Generating webpack stats 5 {"format":"module","expected":"esm"} matches the snapshot 1`] = `
exports[`Generating webpack stats 5 source maps are enabled does not include any source maps 1`] = `
{
"assets": [
{
Expand All @@ -236,7 +236,7 @@ exports[`Generating webpack stats 5 {"format":"module","expected":"esm"} matches
},
],
"builtAt": Any<Number>,
"bundleName": StringContaining "test-webpack-v5-esm",
"bundleName": StringNotContaining ".map",
"bundler": {
"name": "webpack",
"version": "5.90.0",
Expand Down
32 changes: 32 additions & 0 deletions packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,38 @@ const tests: Test[] = [
},
expected: "test.*",
},
{
name: "should replace '[hash:22]' with '*'",
Copy link

@codecov codecov bot Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This test case seems to duplicate the following ones. They all test the same process with the same kind of input, therefore, any error would cause all of them to fail. It would therefore be more efficient to just keep one of them.

input: {
path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

Test cases here (lines 96-125) seem to be over-repetitive; they are all checking the same substitution logic but with different has lengths. Suggest simplifying these.

format: "[name].[hash:22].chunk.js",
},
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

It’s very commendable that test cases have been added for different types and lengths of hashes. I would suggest adding more edge cases, for instance, to handle unexpected hash values or an unusual hash length.

expected: "test.*.chunk.js",
},
{
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

Same as previously, this test case seems to duplicate others. Removing it would lighten the test suite without losing any value.

name: "should replace '[contenthash:22]' with '*'",
input: {
path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",
format: "[name].[contenthash:22].chunk.js",
},
expected: "test.*.chunk.js",
},
{
name: "should replace '[fullhash:22]' with '*'",
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

This is another example of a redundant test that could be removed to make the test suite more efficient.

input: {
path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",
format: "[name].[fullhash:22].chunk.js",
},
expected: "test.*.chunk.js",
},
{
name: "should replace '[chunkhash:22]' with '*'",
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

Likewise, this test case seems to duplicate the previous ones. It tests the same process with similar inputs, therefore, any problematic behavior would cause both of them to fail. We should remove it to lighten the test suite.

input: {
path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",
format: "[name].[chunkhash:22].chunk.js",
},
expected: "test.*.chunk.js",
},
];
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

While the test coverage is good for different kind of hashes, it seems to assume that the hashes will always be of length 3. It would be beneficial to test hashes of different lengths to ensure normalisation works correctly in those scenarios as well.


describe("normalizePath", () => {
Expand Down
14 changes: 5 additions & 9 deletions packages/bundler-plugin-core/src/utils/normalizePath.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
const HASH_REGEX = /[a-f0-9]{8,}/i;
const POTENTIAL_HASHES = [
"[hash]",
"[contenthash]",
"[fullhash]",
"[chunkhash]",
];
const POTENTIAL_HASHES = ["[contenthash", "[fullhash", "[chunkhash", "[hash"];

const escapeRegex = (string: string): string =>
string.replace(/[|\\{}()[\]^$+*?.]/g, "\\$&").replace(/-/g, "\\x2d");
Expand Down Expand Up @@ -33,9 +28,10 @@
leadingDelimiter,
)})`;

const closingBracketIndex = format.slice(match.hashIndex).indexOf("]");
// grab the ending delimiter and create a regex group for it
Copy link

@codecov codecov bot Oct 18, 2024

Choose a reason for hiding this comment

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

Reinitializing endingDelimiter to an empty string here seems unnecessary, as its value is immediately assigned again after this.

Copy link
Member

@AbhiPrasad AbhiPrasad Oct 18, 2024

Choose a reason for hiding this comment

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

I agree - we can do.

let endingDelimiter =
      format.at(...

Copy link
Author

@nicholas-codecov nicholas-codecov Oct 18, 2024

Choose a reason for hiding this comment

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

whoops, my bad. I used to have this in a loop and removed it but forgot to bump it back up.

let endingDelimiter =
format.at(match.hashIndex + match.hashString.length) ?? "";
format.at(match.hashIndex + closingBracketIndex + 1) ?? "";

// If the ending delimiter is `[extname]` there won't be a
// `.<file-extension>` so we need to replace it with a `.` for the
Expand All @@ -48,8 +44,8 @@

// create a regex that will match the hash
// potential values gathered from: https://en.wikipedia.org/wiki/Base64
// added in `\-` to account for the `-` character which seems to be used by Rollup through testing
const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z\/+=-]+)${endingRegex})`;
Copy link

@codecov codecov bot Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the updated regexString, there is a repeated pattern '/+=_/+=-'. This error may affect how the code is interpreted and could cause unexpected behaviors. Please revise this line of code to correct this potential issue.

// added in `\-` and `\_` to account for the `-` `_` as they are included in the potential hashes: https://rollupjs.org/configuration-options/#output-hashcharacters
const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z/\+=_\/+=-]+)${endingRegex})`;

Check failure

Code scanning / CodeQL

Useless regular-expression character escape High

The escape sequence '\+' is equivalent to just '+', so the sequence may still represent a meta-character when it is used in a
regular expression
Loading
.

Copilot Autofix

AI about 1 year ago

To fix the problem, we need to remove the unnecessary escape sequence \+ from the regular expression string. This will make the code cleaner and more readable without changing its functionality.

  • In general terms, we should remove the backslash before the + character in the regular expression string.
  • Specifically, we will edit the regexString on line 48 in the file packages/bundler-plugin-core/src/utils/normalizePath.ts to remove the unnecessary escape sequence.
  • No additional methods, imports, or definitions are needed to implement this change.
Suggested changeset 1
packages/bundler-plugin-core/src/utils/normalizePath.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/packages/bundler-plugin-core/src/utils/normalizePath.ts b/packages/bundler-plugin-core/src/utils/normalizePath.ts
--- a/packages/bundler-plugin-core/src/utils/normalizePath.ts
+++ b/packages/bundler-plugin-core/src/utils/normalizePath.ts
@@ -47,3 +47,3 @@
     // added in `\-` and `\_` to account for the `-` `_` as they are included in the potential hashes: https://rollupjs.org/configuration-options/#output-hashcharacters
-    const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z/\+=_\/+=-]+)${endingRegex})`;
+    const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z/+=_\/+=-]+)${endingRegex})`;
     const HASH_REPLACE_REGEX = new RegExp(regexString, "i");
EOF
@@ -47,3 +47,3 @@
// added in `\-` and `\_` to account for the `-` `_` as they are included in the potential hashes: https://rollupjs.org/configuration-options/#output-hashcharacters
const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z/\+=_\/+=-]+)${endingRegex})`;
const regexString = `(${leadingRegex}(?<hash>[0-9a-zA-Z/+=_\/+=-]+)${endingRegex})`;
const HASH_REPLACE_REGEX = new RegExp(regexString, "i");
Copilot is powered by AI and may make mistakes. Always verify output.
const HASH_REPLACE_REGEX = new RegExp(regexString, "i");

// replace the hash with a wildcard and the delimiters
Expand Down
Loading