js bundle: Try not to minify classnames that end in Error

[chrisbobbe]

Note: I've so far only tested by running yarn start and observing that that doesn't crash.

[chrisbobbe]

Mmm, see my comments below.

[chrisbobbe]

Oops, I think this is wrong. By default, I don't think terser is getting used: facebook/metro#510 (comment), and we're not overriding the default (which we'd do with minifierPath).

Instead, uglify-es is being used. That's unmaintained; terser is a fork. So I'll need to either explicitly use terser with the minifierPath option (means installing terser) or figure out how to do the right config with uglify-es.

A maintainer says this, and I'm not sure what to make of it:

[…] with Hermes we shouldn't need a minifier […]

The maintainer gives that fact (not sure why it's a fact) as a reason to be optimistic about facebook/metro#606 moving forward. The PR makes an API change to let Metro work with a new major version of terser. Facebook reportedly depends heavily on the old API, but can stop doing so when it adopts Hermes more?

But also what does it mean that we don't need a minifier with Hermes? Does Hermes come with a minifier, and can we configure it (like to solve this issue 👀)? Or is minifying just not something you want or need to do when using Hermes?

[chrisbobbe]

Huh, they do recommend disabling minifying when using Hermes; see facebook/hermes#452 (comment):

No, Hermes doesn't need minified input. Minifiers primarily rename local variables and parameters and perform some simple AST transformations, which have very little, if any, impact on the size and performance of a compiled Hermes bundle. We recommend disabling minification with Hermes, but haven't really pushed for it aggressively since the Metro pipeline also supports JSC which does benefit from minification.

[rockwotj]

FWIW you can use the metro-minifier-terser package to switch to minify v4 (which is much better than uglify-js). There is also an extremely hacky version of that package to support terser v5 here

[gnprice]

It sounds like on Hermes what's going on is that you don't end up shipping anything that looks like JS code at all. In this 2021-10 blog post they're explicit about that:

Hermes’s defining feature is how it performs compilation work ahead-of-time, meaning that React Native apps with Hermes enabled ship with precompiled optimized bytecode instead of plain JavaScript source.

So any JS code is just input to that compilation step, at build time, and giving that compiler minified vs. unminified JS doesn't make much difference to the output.

FWIW you can use the metro-minifier-terser package to switch to minify v4 (which is much better than uglify-js).

Thanks for that tip! That seems like a good thing for us to try. (I think you meant to say "terser v4" rather than "minify v4", is that right?)

[chrisbobbe]

Huh, I didn't know that about Hermes. I wonder if that means our current ExtendableError implementation just has no way of working with Hermes.

(our current `ExtendableError` implementation, copy-pasted)

/**
 * `Error`, but subclass instances have the name of the subclass at `.name`
 *
 * All of our custom error classes should extend this. For ones that don't,
 * instances will have "Error" for their `.name`, instead of "TimeoutError",
 * "NetworkError", etc. That's worse than useless because we could
 * misidentify an error while debugging, based on the understandable but
 * wrong assumption that our custom errors' names already correspond to the
 * subclasses' names out of the box.
 */
// TODO: Add linting to make sure all our custom errors extend this
export class ExtendableError extends Error {
  // Careful! Minification might make this differ from what we see in our
  // source code, which would be kind of sad. (I guess we'll find out?)
  // Don't use this for equality checks or user-facing text:
  // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/name#javascript_compressors_and_minifiers.
  //
  // This will work even down a chain of subclasses, i.e.,
  // MyVerySpecificError extends MySpecificError extends ExtendableError. If
  // you call `new MyVerySpecificError(…)`, then `MyVerySpecificError`'s
  // constructor will be the value of `this.constructor` here, so the
  // instance gets the correct name.
  name: typeof Error.name = this.constructor.name;
}

[rockwotj]

(I think you meant to say "terser v4" rather than "minify v4", is that right?)

Yupp sorry about the typo (and the drive by review, but you linked to a PR I was looking at)

[gnprice]

name: typeof Error.name = this.constructor.name;

No, I expect that's fine -- the .name property on a function is a lot more visible than the .toString, and I'd expect Hermes to preserve that.

Googling for a Hermes playground, they have one: https://hermesengine.dev/playground/

Doesn't seem to have direct links to specific demo code (like the handy flow.org/try does), but if you paste the following code there and hit the "play" button:

function a_function() {}
const a_variable = a_function;
print(a_variable.name);

you'll see that "a_function" appears in the resulting bytecode, and "a_variable" doesn't. Then if you edit the options box at the top so it reads just -O (instead of -O -dump-bytecode) and hit the "play" button, you'll see that it prints:

a_function

So that seems to confirm it -- .name on a function (and therefore on a class, which is a special case of a function) is preserved just fine.

[gnprice]

(and the drive by review, but you linked to a PR I was looking at)

No problem at all! I appreciate the tip.

Those cross-project links that GitHub makes can feel noisy sometimes when looking at a PR. But then when they make helpful connections possible like the information you shared, that's what makes them feel worth it 😄

[chrisbobbe]

Revision pushed, this time using terser, after Metro made it possible to use terser v5 in facebook/metro@ed7d5d6 (which we got when we upgraded to RN v0.67).

(It seems like we might not end up enabling Hermes in the React Native app, given its performance problems.)

[gnprice]

Thanks for the revision! And thanks again @rockwotj for the tip above on how to switch to terser.

Looks good; merging, after tweaking the commit message.