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 3 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.123.chunk.js",
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.123.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.123.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.123.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
15 changes: 7 additions & 8 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 @@ export const normalizePath = (path: string, format: string): string => {
)})`;

// 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.

[...format.slice(match.hashIndex)].forEach((char, index) => {
if (char === "]") {
endingDelimiter = format.at(match.hashIndex + index + 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 Down
Loading