Closed jeetiss closed 1 year ago
I tested the bundle size difference for sideEffects: false
separately and it doesn't change anything at all, lol)
so I going to close this PR
This is still worth doing. It's not that it will make this library smaller, it's that if @noble/hashes
ever becomes unreachable in the application of which it's a dependency, that application's bundle will be able to reap the entire thing.
Without sideEffects: false
, the bundler will assume that merely doing import { ... } from '@noble/hashes'
mutates the environment in some important way (which it does not) and will keep all the code.
This is still worth doing. It's not that it will make this library smaller, it's that if @noble/hashes ever becomes unreachable in the application of which it's a dependency, that application's bundle will be able to reap the entire thing.
yes, but @noble/hashes
contains side effects and it is not safe to drop it completely
there is a side effect: https://github.com/paulmillr/noble-hashes/blob/main/src/utils.ts#L27-L32
The side effect being the thrown error?
yes, thrown an error on a module level
If all of @noble/hashes
is dead code in the application that takes as a dependency, I think it would be appropriate not to throw that error, don't you?
So, how should the app behave on unsupported architectures then? Are you suggesting its methods should just corrupt data?
In the case described here the whole library would be reaped by the compiler. There would be no methods left to call.
I don't see how we can have one thing without another. isLE
is side-effect.
Runtime in-hash checks for little-endianness can't be done, because that would screw up perf - which was very thoroughly tested.
Here's an example.
import { bytesToHex } from '@noble/hashes/utils';
export function getRandomBytes() {
const bytes = // Get some random bytes however you like.
if (__DEV__) {
console.log(`Generated random bytes: ${bytesToHex(bytes)}`);
}
return bytes;
}
When your compiler injects true
for __DEV__
you get this compiled output:
import { bytesToHex } from '@noble/hashes/utils';
export function getRandomBytes() {
const bytes = // Get some random bytes however you like.
console.log(`Generated random bytes: ${bytesToHex(bytes)}`);
return bytes;
}
When your compiler injects false
for __DEV__
you get this compiled output:
import { bytesToHex } from '@noble/hashes/utils';
export function getRandomBytes() {
const bytes = // Get some random bytes however you like.
return bytes;
}
Even though every bit of @noble/hashes/utils
became dead code, the compiler did not eliminate it. The reason is because it didn't find "sideEffects": false
in the package.json.
If you add "sideEffects": false
to the package.json, then the compiler will gladly remove the import in the second case.
export function getRandomBytes() {
const bytes = // Get some random bytes however you like.
return bytes;
}
@steveluscher I hear you, but you don't hear me. Adding sideEffects: false
would remove isLE check and its error throwing on unsupported architectures. Yes, it's unfortunate that for some cases it would follow inefficient tree-shaking, but I don't see how the check could be executed otherwise.
I'm trying!
I don't understand why the isLE
check should still run when the app has no need to produce hashes, as in the bottom example I gave.
This is separate from my general inclination for module factories to be side-effect free, which you know by now to predict from me, when you said this:
Runtime in-hash checks for little-endianness can't be done, because that would screw up perf - which was very thoroughly tested.
That's fair! I'd be curious if you could preserve the library's performance characteristics by doing something like this:
function getEndianness() {
return new Uint8Array(new Uint32Array([0x11223344]).buffer)[0] === 0x44
? 'le'
: 'be';
}
let endianness: 'be' | 'le' | undefined;
function hashStuff() {
if ((endianness ||= getEndianness()) !== 'le') {
throw new Error('Non little-endian hardware is not supported');
}
// Produce hash…
}
In any case, just adding "sideEffects": false
would remove the endianness check in the sole case where endianness no longer matters.
In any case, just adding "sideEffects": false would remove the endianness check in the sole case where endianness no longer matters.
As far as I understood, @jeetiss mentioned that adding sideEffects: false
would always remove the isLE variable and the if-error from a bundle. That includes the case when hashes are used.
In short, "sideEffects": false
has zero effect on tree-shakability inside a module, and everything to do with whether a module itself is reapable at the boundary between two modules.
If what you're saying is also true for other bundlers, then I see zero reasons not to add sideEffects: false
to package.json.
Thank you! That's all I came here to achieve.
Do you want to put this PR back up @jeetiss, so that Paul can merge it, or would you like me to?
e37b278
this is my second take on how to make library side effects free. (first one #63)