101arrowz / fflate

High performance (de)compression in an 8kB package
https://101arrowz.github.io/fflate
MIT License
2.27k stars 79 forks source link

Async Gzip throws error saying to use Node 12+ but already running Node 14+ #55

Closed nettybun closed 3 years ago

nettybun commented 3 years ago

How to reproduce

The problem

Hi I'm trying to gzip some files in a .then() promise chain so I tried using gzipAsync() but received the following error:

Error: async operations unsupported - update to Node 12+ (or Node 10-11 with the --experimental-worker CLI flag)
    at Immediate.<anonymous> (file:///home/today/_/work/haptic/node_modules/fflate/esm/index.mjs:34:42)
    at processImmediate (internal/timers.js:456:21)

I'm running Node v14.2.0. The file is ESM via "type": "module" in package.json. The file imports fflate like this:

import esbuild from 'esbuild';
import { gzip, gzipSync } from 'fflate';
import { readFile, writeFile } from 'fs/promises';
// ...

Here's the code that throws on L47:

image

If I replace L47 with res(gzipSync(readData, { consume: true, level: 9 })); then it's fine. It's also fast enough that I'm not reporting an issue with Sync vs Async, just that the error message might be wrong if it's telling people to update to 12+.

Thanks!

101arrowz commented 3 years ago

Actually, all that error really means is "I couldn't require the worker_threads package from the standard library," which I assumed meant that you were in too old a version of Node. However, in this case the issue is probably that dynamic require doesn't exist in an ESM environment. I'll look into a solution, thanks for the report.

nettybun commented 3 years ago

Oh I see, yeah there's a special way to create a require in ESM but changing the error to something like "Couldn't run require('worker_threads'). Are you running Node 12+?" would be fine. I think people trying out ESM-Node are used to errors with require()

101arrowz commented 3 years ago

I've just published v0.6.10, which should fix this error. All I changed was defining require as a local variable with module.createRequire. You should be able to use async operations on Node.js with pure ESM now; let me know if you find any errors.

nettybun commented 3 years ago

Tried it and it works https://github.com/heyheyhello/haptic/commit/1a9c0a20e99bc189f40d1f6737dd8c77dfdff397 Thanks for the patch!

I'll likely keep using gzipSync() though because it's less code and honestly I get confused about the promise+callback+await+worker data flow 😅 I imagine async is probably slower for me only compressing 4 small files? I know async+workers is worth it for large operations but I may have been overkill trying to use it.

I'll close the issue since I don't see any errors :)

101arrowz commented 3 years ago

Yes, if the files are small (say under 300kB) typically the cost of creating a worker thread outweighs the benefit of concurrently compressing all the files at once. So I would suggest sticking with the sync version (but obviously the bug was something that needed to be fixed regardless). Thanks again for the report.