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: search example to support Redis v4+ and Express 4/5 #6274

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vinybk
Copy link

@vinybk vinybk commented Jan 12, 2025

This PR updates the Search example to address issues caused by breaking changes in Redis v4+ and Express 5.

Key changes include:

  • Replacing sadd with sAdd and smembers with sMembers for Redis v4+ compatibility.
  • Using {0,1} syntax for optional parameters instead of ? to support Express 5.
  • Adding error handling for Redis connection and commands.

These changes ensure the example works seamlessly.

Tested locally with:
Redis 7.0.15
Node.js 22.12.0
Express 5.0.1 and 4.21.2
Debian 12

Note:
Adding automated tests for this example could help catch issues like this sooner. If you think it's worthwhile, I’d be glad to help implement them.

@vinybk vinybk changed the title Fix search example to support Redis v4+ and Express 4/5 fix: search example to support Redis v4+ and Express 4/5 Jan 12, 2025
examples/search/index.js Outdated Show resolved Hide resolved
@vinybk vinybk force-pushed the fix-redis-example branch from 6d33482 to 4148e66 Compare January 12, 2025 21:46
…zation into dedicated function to guarantee that it is complete before server starts
@vinybk
Copy link
Author

vinybk commented Jan 13, 2025

Hi @wesleytodd, I've made the requested changes:

  • Updated optional route syntax to /{:query}.
  • Refactored Redis initialization into a dedicated function initializeRedis to make sure that it completes before the server starts.

I think this aligns with your feedback, but please let me know if I misunderstood or missed anything. Thanks!

@vinybk vinybk requested a review from wesleytodd January 13, 2025 18:35
Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. LGTM!

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.

2 participants