Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix imports for ESM modules. #1173

Closed
wants to merge 2 commits into from
Closed

Fix imports for ESM modules. #1173

wants to merge 2 commits into from

Conversation

vorillaz
Copy link

@vorillaz vorillaz commented Apr 4, 2022

Using ESM imports along with Esbuild or Tsup fails on runtime due to default importing the get-port and ioredis modules. This pull request changes the tsconfig file and the failing imports across the BullMQ package in order to play nicely with these bundlers.

Reference: trying to bundle BullMQ failed Esbuild error
Screenshot 2022-04-04 at 1 35 03 PM

@@ -12,8 +12,8 @@
"jsx": "preserve",
"importHelpers": true,
"moduleResolution": "node",
"esModuleInterop": false,
"allowSyntheticDefaultImports": false,
"esModuleInterop": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This setting could have negative effects in other scenarios, there is a risk this could break production build chains if changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @manast
I was a bit concerned regarding the building process but the TSC checks are passing, esbuild and babel are unable to bundle BullMQ tho.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe there are no drawbacks with this setting, I am not sure. I remember battling this setting in the past, but I do not remember the details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is some old discussion, since this flag was actually true in the initial version: #129
Maybe they lured me into changing it :). But the question is, will BullMQ still work for those dependants that have the flag set to false?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, you are right but basically, that's more an issue with ioredis. It seems that one way or another the build will fail for some of us based on the way we are consuming bullmq. Honestly, I think I could live by using bullmq as a common-js module in order to avoid the hustle.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both ioredis@4 and ioredis@5 don't require esModuleInterop to be enabled and it should be able to be imported via import Redis from 'ioredis', but lmk if I'm wrong on this. Happy to make changes if there is anything we can do on ioredis side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luin actually I just tried the following with ioredis version 4.28.5:

import IORedis from 'ioredis';

Building with tsc (3.9.109), gives this error:
image

works with this instead:

import * as IORedis from 'ioredis';

Copy link
Contributor

@manast manast Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem seems to be in the typings, it should say:

export default IORedis

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...yeah that sounds to be an issue on @types/ioredis. ioredis@5 provides official TypeScript declarations so it doesn't have the issue. Probably can create a PR on @types/ioredis but not sure whether it introduces breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to quickly upgrade to version 5 but got tons of errors. Anyway, I think I will wait a couple of months before upgrading.

This was referenced Aug 2, 2022
@boredland
Copy link
Contributor

I took the time in #1346 to perform the needed upgrades. Would be great to get some feedback from @vorillaz, @manast and @luin over there!

@vorillaz vorillaz closed this by deleting the head repository Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants