Skip to content

fix: search example to support Redis v4+ and Express 4/5 - #6274

Merged
bjohansebas merged 3 commits into
expressjs:masterfrom
vinybrun:fix-redis-example
Feb 1, 2026
Merged

bjohansebas merged 3 commits into
expressjs:masterfrom
vinybrun:fix-redis-example

Conversation

@vinybrun

@vinybrun vinybrun commented Jan 12, 2025 •

Copy link
Copy Markdown
Contributor

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.

@vinybrun vinybrun 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
Comment thread examples/search/index.js Outdated
Comment thread examples/search/index.js Outdated
…zation into dedicated function to guarantee that it is complete before server starts
@vinybrun

Copy link
Copy Markdown
Contributor Author

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!

@vinybrun
vinybrun requested a review from wesleytodd January 13, 2025 18:35

@wesleytodd wesleytodd left a comment

Copy link
Copy Markdown
Member

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!

@bjohansebas bjohansebas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I don’t think we want to add more devDependencies, and it seems it was never the intention to add them, those were just examples.

@bjohansebas
bjohansebas merged commit d127723 into expressjs:master Feb 1, 2026
28 checks passed
@UlisesGascon UlisesGascon mentioned this pull request Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants