lz4 check fails in jest

[gabegorelick]

The optional lz4 dependency is currently loaded as follows:

function tryLoadLZ4Module(): LZ4Module | undefined {
  try {
    return require('lz4'); // eslint-disable-line global-require
  } catch (err) {
    const isModuleNotFoundError = err instanceof Error && 'code' in err && err.code === 'MODULE_NOT_FOUND';
    if (!isModuleNotFoundError) {
      throw err;
    }
  }
}

See https://github.com/databricks/databricks-sql-nodejs/blob/56bd0d90d61a2bdff4e62cd55ebcb8d070fa60e2/lib/utils/lz4.ts.

However, this fails when running under Jest due to https://github.com/jestjs/jest/issues/2549 (see also https://github.com/jestjs/jest/issues/11808). Namely, the err instanceof Error check returns false since Jest substitutes its own objects for JS globals. This causes calling code to fail with a ModuleNotFound error.

Is the err instanecof Error check really needed here? Can it be removed?

[kravets-levko]

@gabegorelick Yes, this check is needed because otherwise err has unknown type and TS requires a type assertion before it can be used. Also, there's nothing wrong with this code. The problem comes from Jest messing up globals, and it has to be fixed in Jest.

[haggholm]

@gabegorelick Yes, this check is needed because otherwise err has unknown type and TS requires a type assertion before it can be used.

That doesn't mean it has to be an instanceof check, though. E.g. maybe something like err && typeof err === "object" && "code" in err …

Also, there's nothing wrong with this code. The problem comes from Jest messing up globals, and it has to be fixed in Jest.

I don't really disagree with this; fixing the issue by modifying your library would definitely be going out of your way to make it work with the quirks of certain widely-used tools. But…maybe that's not such a terrible thing? After all it's Jest, not some random test framework written by one guy and used in-house by one small company. It is a workaround for one particular framework, but it's one of the most widely-used, so it's a workaround that in the longer term is likely to help a lot of your users.

[gabegorelick]

That doesn't mean it has to be an instanceof check, though

I would also suggest that something like err as Error may be more appropriate than an instanecof check if the check is truly just to placate Typescript.