fix: Issue when hash length is set #182

nicholas-codecov · 2024-10-18T15:31:58Z

Description

We're currently unable to handle normalizing strings when hashes have a custom length set [hash:16] for example. This PR resolves this issue by searching for the ] character after finding the starting hash string match.

Notable Changes

Update normalizePath to handle custom hash lengths
Update tests

codecov

The changes overall are mostly good, addressing the concern of path normalisation in the event of a custom hash length being set. However, I have concerns about a minor issue in the normalizePath utility and the test cases which could be a bit more effective and efficient.

packages/bundler-plugin-core/src/utils/normalizePath.ts

codecov · 2024-10-18T15:32:28Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

    // grab the ending delimiter and create a regex group for it
-    let endingDelimiter =
-      format.at(match.hashIndex + match.hashString.length) ?? "";
+    let endingDelimiter = "";


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.

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.

codecov · 2024-10-18T15:32:28Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

    expected: "test.*",
  },
+  {
+    name: "should replace '[hash:22]' with '*'",


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.

codecov · 2024-10-18T15:32:28Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+    },
+    expected: "test.*.chunk.js",
+  },
 ];


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.

codecov-notifications · 2024-10-18T15:38:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Components	Coverage Δ
Plugin core	`96.70% <100.00%> (-0.01%)`	⬇️
Rollup plugin	`10.81% <ø> (ø)`
Vite plugin	`11.02% <ø> (ø)`
Webpack plugin	`49.88% <ø> (ø)`

📢 Thoughts on this report? Let us know!

codecov-staging · 2024-10-18T15:43:29Z

Bundle Report

Bundle size has no change ✅

packages/bundler-plugin-core/src/utils/normalizePath.ts

-    // 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})`;
+    // 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})`;


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.

codecov

CodecovAI submitted a new review for 5e6f67c

codecov · 2024-10-18T16:01:45Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+    input: {
+      path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",
+      format: "[name].[hash:22].chunk.js",
+    },


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.

codecov · 2024-10-18T16:01:45Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

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


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.

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.

codecov · 2024-10-18T16:01:45Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

    // 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})`;


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.

codecov

CodecovAI submitted a new review for 275d0f0

codecov · 2024-10-18T16:17:05Z

...ation-tests/fixtures/generate-bundle-stats/webpack/__snapshots__/webpack-plugin.test.ts.snap

      "name": "main-6c1d26e76f6ba1fc75c8.js",
      "normalized": "main-*.js",
      "size": 70961,
    },


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.

codecov · 2024-10-18T16:17:05Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

      leadingDelimiter,
    )})`;

    // grab the ending delimiter and create a regex group for it


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

I agree - we can do.

let endingDelimiter = format.at(...

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

codecov · 2024-10-18T16:17:05Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

+
+    endingDelimiter =
+      format.at(
+        match.hashIndex + format.slice(match.hashIndex).indexOf("]") + 1,


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.

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

codecov · 2024-10-18T16:17:05Z

...gration-tests/fixtures/generate-bundle-stats/rollup/__snapshots__/rollup-plugin.test.ts.snap

      "gzipSize": 98808,
      "name": "main-H2_1FSsQ.js",
-      "normalized": "main-H2_1FSsQ.js",
+      "normalized": "main-*.js",


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

codecov · 2024-10-18T16:17:06Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+  {
+    name: "should replace '[hash:22]' with '*'",
+    input: {
+      path: "test.CoScjXRp_rD9HKS--kYO73.chunk.js",


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.

AbhiPrasad · 2024-10-18T16:30:33Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

      leadingDelimiter,
    )})`;

    // grab the ending delimiter and create a regex group for it


I agree - we can do.

let endingDelimiter = format.at(...

AbhiPrasad · 2024-10-18T16:31:33Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

    // grab the ending delimiter and create a regex group for it
-    let endingDelimiter =
-      format.at(match.hashIndex + match.hashString.length) ?? "";
+    let endingDelimiter = "";


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.

AbhiPrasad · 2024-10-18T16:32:18Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

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


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.

AbhiPrasad · 2024-10-18T16:34:54Z

packages/bundler-plugin-core/src/utils/normalizePath.ts

+
+    endingDelimiter =
+      format.at(
+        match.hashIndex + format.slice(match.hashIndex).indexOf("]") + 1,


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

codecov

CodecovAI submitted a new review for 6f8d670

codecov · 2024-10-18T17:17:16Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+    },
+    expected: "test.*.chunk.js",
+  },
+  {


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

codecov · 2024-10-18T17:17:16Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+    expected: "test.*.chunk.js",
+  },
+  {
+    name: "should replace '[fullhash:22]' with '*'",


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

codecov · 2024-10-18T17:17:16Z

packages/bundler-plugin-core/src/utils/__tests__/normalizePath.test.ts

+    expected: "test.*.chunk.js",
+  },
+  {
+    name: "should replace '[chunkhash:22]' with '*'",


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.

nsdeschenes added 3 commits October 18, 2024 12:28

update normalizePath

615b754

update tests

005813c

add changeset

a815490

codecov bot reviewed Oct 18, 2024

View reviewed changes

small optimization to remove need for looping

dc0e665

also handle underscores

5e6f67c

github-advanced-security bot found potential problems Oct 18, 2024

View reviewed changes

codecov bot reviewed Oct 18, 2024

View reviewed changes

fix some snapshots

275d0f0

codecov bot reviewed Oct 18, 2024

View reviewed changes

AbhiPrasad approved these changes Oct 18, 2024

View reviewed changes

some simple tidy up

6f8d670

codecov bot reviewed Oct 18, 2024

View reviewed changes

remove the ai reviewer

d392379

nicholas-codecov merged commit 0ea4d42 into main Oct 18, 2024
61 of 62 checks passed

nicholas-codecov deleted the fix-issue-when-hash-length-is-set branch October 18, 2024 17:57

@@ -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");

fix: Issue when hash length is set #182

fix: Issue when hash length is set #182

Uh oh!

Conversation

nicholas-codecov commented Oct 18, 2024

Description

Notable Changes

Uh oh!

codecov bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

codecov-notifications bot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov-staging bot commented Oct 18, 2024

Bundle Report

Uh oh!

Check failure

Uh oh!

Copilot Autofix

codecov bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot Oct 18, 2024 •

edited

Loading

codecov-notifications bot commented Oct 18, 2024 •

edited

Loading

codecov bot Oct 18, 2024 •

edited

Loading