Side effects false

[jeetiss]

this is my second take on how to make library side effects free. (first one #63)

[jeetiss]

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

[steveluscher]

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.

[jeetiss]

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

[steveluscher]

The side effect being the thrown error?

[jeetiss]

yes, thrown an error on a module level

[steveluscher]

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?

[paulmillr]

So, how should the app behave on unsupported architectures then? Are you suggesting its methods should just corrupt data?

[steveluscher]

In the case described here the whole library would be reaped by the compiler. There would be no methods left to call.

[paulmillr]

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.

[steveluscher]

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;
}

[paulmillr]

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

[steveluscher]

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.

[paulmillr]

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.

[steveluscher]

Nope! It would not, because it's used in that if() statement.

Compare this: (link)
To this: (link)

[steveluscher]

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.

[paulmillr]

If what you're saying is also true for other bundlers, then I see zero reasons not to add sideEffects: false to package.json.

[steveluscher]

Thank you! That's all I came here to achieve.

[steveluscher]

Do you want to put this PR back up @jeetiss, so that Paul can merge it, or would you like me to?

[paulmillr]

e37b278