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

Feat(medusa): Replace redis with ioredis + redis_options config module #2437

Closed
wants to merge 2 commits into from
Closed

Conversation

alacroix
Copy link

@alacroix alacroix commented Oct 13, 2022

What

Use only one Redis client (ioredis) in medusa package instead of having ioredis + redis simultaneously.
Add also a redis_options field in the project config module to easily configure advanced options for Redis.

Why

When deploying Medusa in production with Redis, I encountered the problem that Redis clients doesn't work well by default with IPv6 addresses.

redis/ioredis#1576
redis/node-redis#1550

Medusa being on top of theses clients, we need to be able to configure it while staying backward compatible.

How

Replaced redis used in loaders/express.ts with ioredis. connect-redis used to store Express session is compatible with both clients. Maybe I'm missing an historic issue with the choice of double Redis clients.
Add second argument for all new Redis with redis_options (typed with the interface RedisOptions from ioredis now that we have only one client). If no defined, will do nothing so backward compatible.

Example:

// in medusa-config.js
module.exports = {
    ...
    redis_url: "redis://localhost:6379";
    redis_options: { family: 6 };
    ...
}
...
const redisClient = new Redis(
      configModule.projectConfig.redis_url,
      configModule.projectConfig.redis_options // if undefined, arg is ignored by ioredis
)

Testing

Don't know exactly how to test it in a proper way. I used patch-package on my project to have the changes working. Any suggestions welcomed :)

@alacroix alacroix requested a review from a team as a code owner October 13, 2022 09:58
@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2022

⚠️ No Changeset found

Latest commit: 9295e65

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sslotsky
Copy link

Big hopes for this as I believe it would solve my issue of being unable to connect to Redis in a Fly.io app! 🙏🏼

@alacroix alacroix closed this Apr 17, 2023
@alacroix alacroix deleted the develop branch April 17, 2023 16:02
@durdenx
Copy link

durdenx commented May 30, 2023

@sslotsky have you found a solution?

@sslotsky
Copy link

@sslotsky have you found a solution?

@durdenx sort of 😂 I actually wound up going with Vendure instead

@durdenx
Copy link

durdenx commented May 31, 2023

I've found a solution for passing redis options with query strings : redis://user:pass@fly-project-redis.upstash.io/?family=IPv6

@sslotsky
Copy link

Nice! I was only just starting my project at the time so I didn't have much at all invested in Medusa. At this point I'm pretty thoroughly invested in Vendure but it's good to know that this is an option in case I want to try Medusa for something else.

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.

None yet

3 participants