Skip to content

Feat/hash serialization #1382

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 2 commits into from
Oct 9, 2021
Merged

Feat/hash serialization #1382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aba706d
feat: generate more collision resistant localIdentHash'es:
subzey Oct 1, 2021
725e4a5
feat: generate more collision resistant localIdentHash'es:
subzey Oct 7, 2021
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
31 changes: 19 additions & 12 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,21 +359,28 @@ function defaultGetLocalIdent(
);
}

// eslint-disable-next-line no-underscore-dangle
const hash = loaderContext._compiler.webpack.util.createHash(hashFunction);
const { hashSalt } = options;
let localIdentHash = "";
for (let tier = 0; localIdentHash.length < hashDigestLength; tier++) {
// eslint-disable-next-line no-underscore-dangle
const hash = loaderContext._compiler.webpack.util.createHash(hashFunction);
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid only for perf here, create multiple times hash is not good idea, can you explain why we need create it again and again and do not reuse existing hash?

Copy link
Contributor Author

@subzey subzey Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The 2nd iteration is only required if the digest length is not enough to generate a pseudorandom string long enough.

If we won't change anything between the iterations then each iteration would generate the same output over and over and the string would just cycle:

If hash(str) = "ABCDE", then it would be "ABCDEABCDEABCDEA". Even if this str is 16 chars long its "uniqueness" is only 5 chars long.

I've ran npm test:only logging the number of times this loop iterated per function call.

1375 calls only ran this loop once.

122 twice.

2 calls took three iterations

Copy link
Contributor Author

@subzey subzey Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Upd: My bad, jest spawns 4 processes and they were concurrently writing the same file 😓

1672 - 1 time
125 - 2 times
2 - 3 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've investigated those two suspicious calls where the loop is iterated 3 times.

It's my own xxhash64 test from this very PR. I'm asking a hex digest of length 20 (= 80bits) from a digest that can only produce 64 bits and have a really high chance to produce a long run of leading digits.

Iteration 0:
digest is 7268299549203d4d
Leading digits are dropped, the resulting string so far is:
7268299549203 d4d

Iteration 1:
digest is 26a899717294ba28 =>
7268299549203 d4d 26a899717294ba28

Iteration 2:
digest is 45400c2599876c30. The string is trimmed at length 20:
7268299549203 d4d 26a899717294ba28 4 5400c2599876c30

const { hashSalt } = options;
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this under loop, because we don't change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 725e4a5


if (hashSalt) {
hash.update(hashSalt);
}
if (hashSalt) {
hash.update(hashSalt);
}

const tierSalt = Buffer.allocUnsafe(4);
tierSalt.writeUInt32LE(tier);
hash.update(tierSalt);

hash.update(options.content);
hash.update(options.content);

const localIdentHash = hash
.digest(hashDigest)
.slice(0, hashDigestLength)
.replace(/[/+]/g, "_")
.replace(/^\d/g, "_");
localIdentHash = (localIdentHash + hash.digest(hashDigest))
.replace(/^\d+/, "")
.replace(/\//g, "_")
.replace(/\W+/g, "")
Copy link
Member

Choose a reason for hiding this comment

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

And I am afraid about these changes, can you explain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drop all leading digits
Then replace any / with '_'
Then drop anything except a-zA-Z0-9_ (including + and '=' of base64)

I could change the last one to /[^a-zA-Z0-9_]+/, for better readablility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 725e4a5

.slice(0, hashDigestLength);
}

// TODO need improve on webpack side, we should allow to pass hash/contentHash without chunk property, also `data` for `getPath` should be looks good without chunk property
const ext = path.extname(resourcePath);
Expand Down
16 changes: 8 additions & 8 deletions test/__snapshots__/esModule-option.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,10 @@ var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(___CSS_LOADER_API_NO_S
___CSS_LOADER_EXPORT___.i(___CSS_LOADER_AT_RULE_IMPORT_0___);
var ___CSS_LOADER_URL_REPLACEMENT_0___ = ___CSS_LOADER_GET_URL_IMPORT___(___CSS_LOADER_URL_IMPORT_0___);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.Yz6vxyapD7cLc0x63wym {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.OZJqogC5EaF_wROug7zE {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
// Exports
___CSS_LOADER_EXPORT___.locals = {
\\"class\\": \\"Yz6vxyapD7cLc0x63wym\\"
\\"class\\": \\"OZJqogC5EaF_wROug7zE\\"
};
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -168,7 +168,7 @@ exports[`"esModule" option should work with a value equal to "true" and the "mod
Array [
Array [
"../../src/index.js??ruleSet[1].rules[0].use[0]!./es-module/imported.css",
".rJenlm2lw3fBBPGxPidc {
".EB7DBFwH4lzwZcKIj2OA {
color: red;
}
",
Expand All @@ -180,7 +180,7 @@ Array [

/* Comment */

.Yz6vxyapD7cLc0x63wym {
.OZJqogC5EaF_wROug7zE {
color: red;
background: url(replaced_file_protocol_/webpack/public/path/img.png);
}
Expand All @@ -205,10 +205,10 @@ var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(___CSS_LOADER_API_NO_S
___CSS_LOADER_EXPORT___.i(___CSS_LOADER_AT_RULE_IMPORT_0___);
var ___CSS_LOADER_URL_REPLACEMENT_0___ = ___CSS_LOADER_GET_URL_IMPORT___(___CSS_LOADER_URL_IMPORT_0___);
// Module
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.Yz6vxyapD7cLc0x63wym {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
___CSS_LOADER_EXPORT___.push([module.id, \\"@charset \\\\\\"UTF-8\\\\\\";\\\\n\\\\n/* Comment */\\\\n\\\\n.OZJqogC5EaF_wROug7zE {\\\\n color: red;\\\\n background: url(\\" + ___CSS_LOADER_URL_REPLACEMENT_0___ + \\");\\\\n}\\\\n\\", \\"\\"]);
// Exports
___CSS_LOADER_EXPORT___.locals = {
\\"class\\": \\"Yz6vxyapD7cLc0x63wym\\"
\\"class\\": \\"OZJqogC5EaF_wROug7zE\\"
};
export default ___CSS_LOADER_EXPORT___;
"
Expand All @@ -218,7 +218,7 @@ exports[`"esModule" option should work with a value equal to "true" and the "mod
Array [
Array [
"../../src/index.js??ruleSet[1].rules[0].use[0]!./es-module/imported.css",
".rJenlm2lw3fBBPGxPidc {
".EB7DBFwH4lzwZcKIj2OA {
color: red;
}
",
Expand All @@ -230,7 +230,7 @@ Array [

/* Comment */

.Yz6vxyapD7cLc0x63wym {
.OZJqogC5EaF_wROug7zE {
color: red;
background: url(replaced_file_protocol_/webpack/public/path/img.png);
}
Expand Down
Loading