-
Notifications
You must be signed in to change notification settings - Fork 418
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: modified imports to work when esModuleInterop is disabled #132
Conversation
src/classes/redis-connection.ts
Outdated
@@ -1,14 +1,14 @@ | |||
import { EventEmitter } from 'events'; | |||
import IORedis, { Redis } from 'ioredis'; | |||
import * as Redis from 'ioredis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
src/classes/job.ts
Outdated
@@ -1,4 +1,4 @@ | |||
import IORedis from 'ioredis'; | |||
import * as Redis from 'ioredis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't we just do:
import { Redis } from 'ioredis'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will work for all cases where it's used as an interface. However, this usage won't work (or at least I can't figure out how to make it work):
new Redis();
'Redis' only refers to a type, but is being used as a value here.ts(2693)
I'm not sure why the ioredis typings are written this way.
The import in this particular file, src/classes/job.ts
, could be changed to this since there are no instantiations here:
import { Redis, Pipeline } from 'ioredis';
I used the same import style across all of the files for consistency and to account for files that need to instantiate an instance. I suppose it's a matter of opinion. I could change all of the files that don't instantiate Redis to the named import style if you'd prefer.
This reply should apply to all of your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but in many places we are not instantiating, just using it as an interface. Besides that, I think that we should change the name of the import to IORedis to make it explicitly that it is the module name, so it will become IORedis.Redis instead of the slightly more awkward Redis.Redis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. I will make those changes.
src/classes/worker.ts
Outdated
import { Redis } from 'ioredis'; | ||
import path from 'path'; | ||
import * as fs from 'fs'; | ||
import * as Redis from 'ioredis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
src/commands/index.ts
Outdated
@@ -12,7 +12,7 @@ | |||
*/ | |||
'use strict'; | |||
|
|||
import IORedis from 'ioredis'; | |||
import * as Redis from 'ioredis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
src/interfaces/queue-options.ts
Outdated
@@ -1,6 +1,6 @@ | |||
import { JobsOptions } from '../interfaces'; | |||
|
|||
import IORedis from 'ioredis'; | |||
import * as Redis from 'ioredis'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/test/test_clean.ts
Outdated
@@ -21,7 +21,7 @@ describe('Cleaner', () => { | |||
afterEach(async function() { | |||
await queue.close(); | |||
await queueEvents.close(); | |||
await removeAllQueueData(new IORedis(), queueName); | |||
await removeAllQueueData(new Redis(), queueName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its late now, so I am confused why we can instantiate Redis here and not just import the Redis class by this (as commented above)
import { Redis } from 'ioredis'
🎉 This PR is included in version 1.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This change should enable compatibility with top-level projects that have esModuleInterop disabled.
I did not include any tests since I disabled esModuleInterop in BullMQ. This should cover any future issues.
Fixes GH-129.