Implement fallback hashing algorithm when crypto module is not available #19941
Conversation
|
|
src/compiler/sys.ts
Outdated
| } | ||
| else { | ||
| // djb2 hashing algorithm: http://www.cse.yorku.ca/~oz/hash.html | ||
| const chars = data.split("").map(str => str.charCodeAt(0)); |
There was a problem hiding this comment.
Can you make this a exported function with /*@internal*/ and also use it at https://github.com/Microsoft/TypeScript/blob/master/src/compiler/watch.ts#L645 in the system.createHash undefined case.
|
Thanks for the addition @bttf , I've been lazy on this one. Just looking at the original comments, one place concerns me: https://github.com/Microsoft/TypeScript/blob/0d9bc55033a3b5c8f68622803409476c7a6f82b4/src/compiler/sys.ts#L64 Why does it say 'should be cryptographically secure'? I did raise this question:
reply:
Expectation is not 100% the same confidence as knowing for sure. Looking at the usage in watch.ts the answer isn't that clear. @sheetalkamat you've put lots of work in there, can you clarify please? How does the logic in Many thanks! |
|
@mihailik The hash is created from .d.ts(so that we know shape of the module). If hash1 corresponds to text1 and hash2 corresponds to text2 then when shape of the module changes from text1 to text2, if hash1 = hash2 it would mean that we wont emit any other modules that depend on (import directly or indirectly). |
|
@sheetalkamat that backtracks on previous clarification by @mhegazy. And it implies the current released TSC is guaranteed to produce incorrect build results randomly (albeit rarely). Also the risk with the weak hash is higher. Should the weak hash implmentation instead return the argument as result, testing harness does that already? If they don't have full crypto, it's going to be slower - but not crash, neither produce broken builds. |
src/compiler/sys.ts
Outdated
| * http://www.cse.yorku.ca/~oz/hash.html | ||
| */ | ||
| /* @internal */ | ||
| export function generateDjb2Hash(data: string): string { |
There was a problem hiding this comment.
no need for this to be exported i would say. just make createHash always implemented in sys.
src/compiler/sys.ts
Outdated
| @@ -494,9 +511,16 @@ namespace ts { | |||
| } | |||
| }, | |||
| createHash(data) { | |||
There was a problem hiding this comment.
consider defining two functions instead:
function createMD5HashUsingNativeCrypto(data: string) {
const hash = _crypto.createHash("md5");
hash.update(data);
return hash.digest("hex");
}
function createDjb2Hash(data) {
const chars = data.split("").map(str => str.charCodeAt(0));
return `${chars.reduce((prev, curr) => ((prev << 5) + prev) + curr, 5381)}`;
}then in here make createHash either one or the other based on the existance of _crypto,e.g.:
{
...
createHash: _crypto ? createMD5HashUsingNativeCrypto : createDjb2Hash
...
}
src/compiler/watch.ts
Outdated
| @@ -642,7 +642,7 @@ namespace ts { | |||
| } | |||
|
|
|||
| function computeHash(data: string) { | |||
| return system.createHash ? system.createHash(data) : data; | |||
| return system.createHash ? system.createHash(data) : generateDjb2Hash(data); | |||
There was a problem hiding this comment.
just use createHash when available. you already made it use generateDjb2Hash in sys. watch is no triggered except on node anyways.
|
@sheetalkamat what about @mihailik suggestion of just returning the string.. it is definitely more accurate.. |
|
We can use same string but then it also means that huge string is stored in cache and would be there for each output for life time (till it changes to something else) (in tsc --w or compile on save from editor) |
|
good point. |
|
@bttf i think this change i ready to go. i am just in the middle of snapping for TS 2.7. i will merge this once that is done. |
Improves upon #19101; Fixes #19100