Skip to content

Conversation

@mgravell
Copy link
Collaborator

@mgravell mgravell commented Jun 13, 2023

fixes #2479 (option 1)

get => s_DefaultPatternMode == PatternMode.Auto;
set => s_DefaultPatternMode = value ? PatternMode.Auto : PatternMode.Literal;
}
private static PatternMode s_DefaultPatternMode = PatternMode.Auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a property to ConfigurationOptions to control this default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can't; ConfigurationOptions is per-instance, this is global; existing code like (RedisChannel)"abc" is not aware of any config model, so would have nowhere to get the info from, and if we try to make ConfigurationOptions touch the global setting, then it will be "last model wins", which is ultra unfriendly; at least the global nature is clear

mark the implicit RedisChannel operators as [Obsolete]
@mgravell mgravell merged commit 8abe002 into main Jun 13, 2023
@mgravell mgravell deleted the auto-pattern branch June 13, 2023 15:11
@arteny
Copy link

arteny commented Jun 14, 2023

You marked string initialization of RedisChannel as Obsolete, but there is no any doumentation how to use new RedisChannel functions.

@mgravell
Copy link
Collaborator Author

The obsolete message literally gives that guidance, but yes we should check for any existing docs and update

@arteny
Copy link

arteny commented Jun 14, 2023

The obsolete message literally gives that guidance, but yes we should check for any existing docs and update

A guidance? It raises questions only.
Warning CS0618 'RedisChannel.implicit operator RedisChannel(string)' is obsolete: 'It is preferable to explicitly specify a PatternMode, or use the Literal/Pattern methods'

What is PatternMode and Literal/Pattern methods?

@mgravell
Copy link
Collaborator Author

I will revise the message, and improve the docs, but to give you an answer today: RwdisChannel.Literal / RedisChannel.Pattern, or the RedisChannel constructor that takes a PatternMode

@arteny
Copy link

arteny commented Jun 14, 2023

ok, understood now, so instead of providing string key for subscribe, we need to use RedisChannel.Literal(key) or RedisChannel.Pattern(keysPattern) if using patterns

@mgravell
Copy link
Collaborator Author

You can still use a string if you eat the CS0618 - and if you want to use the old behaviour without the warning, the constructor (explicitly stating .Auto as the enum) achieves that. It is ambiguous though, and might not be advisable in all scenarios. The design is regretted, and we would prefer to make people ask themselves "am I subscribing to a single literal channel, or to a pattern of channels?"

@jeffputz
Copy link

jeffputz commented Sep 9, 2023

Just getting around to a package update. This obsolete message still completely lacks context, and all I've been able to find is this PR, which is also absent intent of the change.

@mgravell
Copy link
Collaborator Author

mgravell commented Sep 9, 2023

I'll try to improve that. Short version:

  • if you're publishing: use RedisChannel.Literal
  • if you're subscribing, think about whether you intend to allow wildcards, and use Literal for single channel or Pattern if you intend that

@jeffputz
Copy link

jeffputz commented Sep 9, 2023

@mgravell that makes sense, thank you. I think the missing context was that, until this, I wasn't aware that wildcards were possible in the pub/sub, so that's something new to me. I can't think of any situations where I would want to do that, but I suppose someone else could.

@mgravell
Copy link
Collaborator Author

mgravell commented Sep 9, 2023

Right; so use RedisChannel.Literal - job done

@joshbartley
Copy link

One part of the missing doc update is the redis backplane docs which use the obsolete string method.

https://learn.microsoft.com/en-us/aspnet/core/signalr/redis-backplane?view=aspnetcore-8.0

@mgravell
Copy link
Collaborator Author

I can take that, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Suggested analyzer re channel subscribe

6 participants