geotiffjs / geotiff.js

geotiff.js is a small library to parse TIFF files for visualization or analysis. It is written in pure JavaScript, and is usable in both the browser and node.js applications.
https://geotiffjs.github.io/
MIT License
878 stars 183 forks source link

v2.0.5 is a breaking change for CommonJS users #301

Open MichaelBuhler opened 2 years ago

MichaelBuhler commented 2 years ago

I think the code in v2.0.5 is a breaking change for CommonJS users, such as my organization.

The underlying cause is because of the quick-lru dependency that was added is ESM only.

I guess more broadly, anyone who is requireing geotiff in a CommonJS package with see a failure with geotiff@2.0.5 because Node cannot require the quick-lru module in geotiff.js/dist-module/source/blockedsource.js around line 7. They will see an error message, such as:

/path/to/code/node_modules/geotiff/dist-node/source/blockedsource.js:7
const quick_lru_1 = __importDefault(require("quick-lru"));
                                    ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /path/to/code/node_modules/quick-lru/index.js from /path/to/code/node_modules/geotiff/dist-node/source/blockedsource.js not supported.
Instead change the require of index.js in /path/to/code/node_modules/geotiff/dist-node/source/blockedsource.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/path/to/code/node_modules/geotiff/dist-node/source/blockedsource.js:7:37)
    at Object.<anonymous> (/path/to/code/node_modules/geotiff/dist-node/source/remote.js:6:28)
    at Object.<anonymous> (/path/to/code/node_modules/geotiff/dist-node/geotiff.js:33:21)
    ... {
  code: 'ERR_REQUIRE_ESM'
}

Not sure how to proceed here, maybe a warning should be added to release notes or something.

Also (I don't have a lot of knowledge in this area), is there any advantage to building and distributing the dist-node folder if you have to use ESM imports now?

Another option is to downgrade quick-lru to v5.1.1 as the ESM breaking change is in v6.0.0. See here.

Refactoring my whole app to "type": "module" probably can't happen soon, so I will miss out on some good features slated for the next release (v2.0.6 perhaps).

DanielJDufour commented 2 years ago

I ran into this issue as well. I use geotiff as a nested dependency in geomask. It's far from a workaround for general use, but I got around this issue by basically returning an empty object from require when it hits an ESM Import error. I'm providing the solution here in case it is enough for your use case:

see https://github.com/DanielJDufour/geomask/blob/main/require-esm-as-empty-object.js:

const Module = require("module");

const __require__ = Module.prototype.require;

Module.prototype.require = function () {
  try {
    return __require__.apply(this, arguments);
  } catch (error) {
    if (error.message.includes("require() of ES Module")) {
      return {};
    } else {
      throw error;
    }
  }
};

I too am hoping for a more general solution because I use geotiffjs in tens of small libraries and converting everything to ES6 would be a pain (as well as potentially causing issues for other users).

fwiw, I've had some success in the past converting ESM syntax to CJS syntax doing string replaces (replacing export default with module.exports =, but it's rather brittle.

Sorry I can't be more helpful, but I'll try to brainstorm other solutions, too.

DanielJDufour commented 2 years ago

I published the code above in its own npm module (https://github.com/DanielJDufour/require-esm-as-empty-object), so you can do: node -r require-esm-as-empty-object app.js

twelch commented 2 years ago

Did you try importing geotiff using a dynamic import as the error suggests? Not sure if this handles converting all underlying dependencies too. Something like the following you might be able to put into your own wrapper module:

const geotiff = await import('geotiff');
export default geotiff

(Edit) Hopefully you read it as an inquisitive question, not accusatory 😄 I'm actually curious if this will work. I've read that dynamic import is "the" bailout for CJS modules to be able to use esm modules. And I'm not sure what its limits are.

DanielJDufour commented 2 years ago

@twelch , that's a great suggestion. I haven't tried it yet, but that would be a awesome :-)

fwiw, I'm running into the same error when I'm running ts-node.

twelch commented 2 years ago

@DanielJDufour @MichaelBuhler here's some quick tests in node16 and ts-node that seem to work. Note the jsFail script that reproduces the error, and index.js is successful - https://github.com/twelch/geotiff-dynamic-import-test

For ts-node I had to do a little something extra - https://github.com/TypeStrong/ts-node/discussions/1290.

DanielJDufour commented 2 years ago

I like @twelch 's solutions, but if you want a quick bandaid to get things working in test (but not production), I published require-esm-deasync, which you can use to get quick-lru required synchronously. I use geotiff.js in a lot of the setup parts of test scripts and didn't want to refactor more than updating a node test.js to node -r require-esm-deasync test.js

DanielJDufour commented 2 years ago

I'm using it here: https://github.com/GeoTIFF/geotiff-precise-bbox/blob/main/package.json#L18

    "test": "node -r require-esm-deasync ./test.js",
ashmortar commented 7 months ago

I am unable to get any of these solutions to work for me.

zhanghjtx commented 6 months ago

so has anyone solved this question