fix(processor): support ESM processor

[imathews]

Hi! I was attempting to use sandboxed processes in an ESM project, and was running into issues as described in #924 . This change seems to resolve the issue for me, and I believe should work for both ESM and CJS projects.

[manast]

Thanks for the PR, unfortunately, it does not seem to work with the current tests...

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[imathews]

Hi @manast - it looks like this was failing eslint tests because the eslint parserOptions didn't support dynamic imports. If we bump eslint parserOptions to ecmaVersion: 2020 it looks like those tests run fine.

I also realized that dynamic imports aren't supported in node < 14 (and I believe this version of Bull supports back to v10), so I added a check to fallback to the previous require syntax for old versions of node. Hopefully this seems reasonable - with more and more libraries pushing ESM in the node ecosystem, it's becoming increasingly necessary for us to be able to write our processors in ESM.

[manast]

Recently I had a lot of trouble using ESM on a sandboxed processor, so I wrote a blog post explaining the only way I managed to use typescript with BullMQ https://blog.taskforce.sh/using-typescript-with-bullmq/ So your change would actually incur on a breaking change, I wonder if you have some example code that illustrates how to use it in practice?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[JiiHu]

@imathews Any news on this? I would love to use this in production 😄

[VladimirMikulic]

Hi @manast. I started migrating a codebase from CJS to ESM and realized that ESM isn't supported in Bull. This change by @imathews is 100% backwards compatible + allows projects to transition to ESM.

Dynamic imports started being available with Node.js 13 (14 for LTS). It is worth noting that dynamic import is an expression that can import both CJS and ESM code.

So if Node.js version is less than 13, the if semaphore will use require to load worker. On the other hand, if Node.js version is greater than or equal to 13, we know that dynamic import expression is available and it will load just fine both CJS and ESM.

This is not a breaking change. Legacy projects will still be able to upgrade BullMQ to benefit from latest features and fixes while new projects can be started in ESM.

Let me know if this explanation makes sense and if you need anything more. I'd really appreciate if you could merge these few magic lines :)

[manast]

We have tried several times with such updates and always ends up breaking for some users. So in theory it is not a breaking change, as many times before, but in practice, it will be. I have merged and reverted many such changes before.

[manast]

Note, I wish NodeJS worked better in this regard, I am frustrated as everybody else.

[VladimirMikulic]

We have tried several times with such updates and always ends up breaking for some users. So in theory it is not a breaking change, as many times before, but in practice, it will be. I have merged and reverted many such changes before.

@manast Could you please provide me with some links to your past attempts? I'd more than happy to investigate them and explain each.

Note, I wish NodeJS worked better in this regard, I am frustrated as everybody else.

I definitely agree. NodeJS is fragmented but I think we can make it for Bull :)

[manast]

This is the latest attempt that I had to revert (in BullMQ but the code is basically the same): https://github.com/taskforcesh/bullmq/pull/1384

[imathews]

@manast I think the notable difference in this PR (compared to https://github.com/taskforcesh/bullmq/pull/1384) is that it performs a node version check before using any import semantics. So I can't imagine this would have any effect for users on previous versions of Node, and on more recent versions this code should be fully cross-compatible between ESM and CJS.

[VladimirMikulic]

Yes, you're right @imathews. await import is an expression so having that line in an environment that doesn't support dynamic imports is fine - as long as the statement is not reached. (the reason for if check)

The ESM issue mentioned in this https://github.com/taskforcesh/bullmq/issues/917 wasn't actually caused by https://github.com/taskforcesh/bullmq/pull/1384. The root cause was improper addition of exports field in package.json that this PR reverted https://github.com/taskforcesh/bullmq/pull/1388

[manast]

Maybe you are right, but, I am not sure it is so simple. Depending on your package settings, Node could complain also, for example, if you are not using the "type: module" and try to use import in some version of NodeJS... so in order to be sure it would be needed to test all the combinations of the supported node versions and the package.json settings that affect the import behavior.

[VladimirMikulic]

@manast await import is an expression that can be used in CJS context. (i.e. without `"type": "module" being set)

[VladimirMikulic]

https://nodejs.org/api/esm.html#import-expressions "Dynamic import() is supported in both CommonJS and ES modules. In CommonJS modules it can be used to load ES modules."

[VladimirMikulic]

For anyone else browsing the thread and who also has to migrate the codebase to ESM, you can use this as a workaround:

In your package.json (for NPM; if you're on yarn 1.x check out patch-package)

  "scripts": {
    "postinstall": "sed -i 's/require(processorFile)/await import(processorFile)/g' ./node_modules/bullmq/dist/cjs/classes/child-processor.js"
  }

Now you can use ESM in your sandboxed worker file:

import doSomeWork from "./myfile.js";

export default (job) => {
  console.log("Job received", job);
  doSomeWork(job);
}

[jonaskello]

Also migrating a codebase from CJS to ESM and hit this problem. However I'm hesitant to do the monkey patch with sed :-). Could this be re-opened please? If breaking change is the only concern then could this just be put behind some kind of flag to fully keep old behavior as default? Like check an ENV variable BULL_ESM=true?

[manast]

I don't think a flag is sufficient as Node itself can complain in some scenarios, but maybe I am wrong. The problem here is that this has bitten us so many times in the past, I am kind of skeptical it can be solved without ruining it for others.

[VladimirMikulic]

@manast I covered the first failed rollout in https://github.com/OptimalBits/bull/pull/2341#issuecomment-1340171274. If you have some more examples from history, I'd be happy to dive in and find what went wrong. It's also worth mentioning that we've been using my sed workaround for almost three months now in production without any issues.

[Riveer01]

Hello @VladimirMikulic , I am trying to find the path you mentioned below, but did not find bullmq, since I am using bull 4.10.4, is there anything missing? Any help is highly appreciated. ./node_modules/bullmq/dist/cjs/classes/child-processor.js

"scripts": { "postinstall": "sed -i 's/require(processorFile)/await import(processorFile)/g' ./node_modules/bullmq/dist/cjs/classes/child-processor.js" }

[VladimirMikulic]

@Riveer01 I use bullmq so for me the path includes bullmq, for you it's bull.

[Riveer01]

@VladimirMikulic  , Thanks for your kind help. I checked the related path, seems there is no file named child-processor.js in bull, could you please tell which files are the counterparts in bull? Thanks so much again.

[VladimirMikulic]

Ok, so for Bull it should be:

  "scripts": {
    "postinstall": "sed -i 's/require(msg.value)/await import(msg.value)/g' ./node_modules/bull/lib/process/master.js"
  }

[Riveer01]

Hello @VladimirMikulic , I have modified the master.js file, but there are still errors as below: Error loading process file /mnt/sfs/project/service/processor.js. Cannot use import statement outside a module, job:[object Object] processor.js is exactly the same as yours:

import doSomeWork from "./myfile.js";

export default (job) => {
  console.log("Job received", job);
  doSomeWork(job);
}

myfile.js:

const fs = require('fs');

export default function doSomeWork () {
  fs.writeFileSync('/mnt/sfs/project/service/222.txt', '555');
}

The bull version is 4.10.4, and need your further help, thanks a lot.

[VladimirMikulic]

@Riveer01 bull package differs quite a lot from bullmq. I personally don't use bull. I'd recommend you switching to bullmq which is successor to bull.

[Riveer01]

@VladimirMikulic , Today I have switched to bullmq, but still got below error. BTW, I have modified the file child-processor.js you metioned, and also used esm in the project, will this cause problems regarding this issue?

// new Worker
const videoWorker = new Worker(`VideoQueue`, path.join(__dirname, 'processor.js'), {connection: videoRedisConnection});

// processor.js
import { SandboxedJob } from 'bullmq';

export default (job) => {
  console.log("Job received", job);
  // await job.log("Start processing job");
}

[VladimirMikulic]

@Riveer01 make sure to set "type": "module" in your package.json.

[Riveer01]

@VladimirMikulic , I have tried to set "type": "module" in the package.json of my project, but there is still error. I have also tried to set "type": "module" in bullmq's package.json, still error.

[Riveer01]

@VladimirMikulic

I followed your way and recreated a small project, and this time it worked! Thank you for your help!