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 5 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
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
19 changes: 9 additions & 10 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 @@ -34,8 +29,12 @@
)})`;

// 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) ?? "";
let endingDelimiter = "";
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 logic could be optimized. Currently you are slicing the string then iterating over the resulting characters. An alternate approach could be to directly find the next closing bracket starting at hashIndex in the string and then getting the following char for delimiter. This could save iterations.

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.

Also correct, but not worth the performance win because of the size of string, array lookups are cheap because the n is small for this method in general.

Only worry is the extra allocations, but considering this should be immediately GC'd for the most part not a problem.

In general the optimization would be to do this in single pass, and explicitly iterate over format and collect the delimiters accordingly.


endingDelimiter =
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 this code change 'format.at(match.hashIndex + format.slice(match.hashIndex).indexOf("]") + 1)' correctly finds the ending delimiter by adding 1 to the index of the matching hash, it might be easier to understand and more efficient if the '.indexOf("]")' portion is performed once outside of the function for reuse.

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 also agree codecov bot :)

format.slice(match.hashIndex).indexOf("]") become it's own variable helps readability of why endingDelimiter was calculated in this way. I would extract it out.

format.at(
match.hashIndex + format.slice(match.hashIndex).indexOf("]") + 1,
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 index slicing here could be prone to out-of-bounds errors. Consider adding a safety check to ensure that index used in at() is less than format.length.

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.

slice doesn't throw errors codecov bot, you're thinking of the wrong language.

) ?? "";

// 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 +47,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