Avoid static state; Mapper registry managed through mapper configuration #1848
Conversation
|
This will break functionality in AutoMapper.Collections because it has the collection profile add 2 object mappers in specific locations in the mapper configuration list. I understand this is technically better implementation in that the list resets on every mapper configuration created, but I'm curious is this a problem for people. |
|
See the issue. You just have to build the list yourself. |
|
I don't think #1847 has anything to do with this. Could be wrong though. |
|
Ok I see what you mean now. But alternatively can you make the mappers themselves thread safe as an alternative? That way there's no need to new up new classes every time? @jbogard what do you think? |
|
There is no need to share them. They are created only when a new configuration is created. |
|
Only thing I can think of is if you make ConvertMapper thread safe, you save on having to re-initialize Convert functions for unit tests, saving time generating expressions over and over again for each test. |
|
Actually they are lazy now. |
|
They are lazy but if you are using a lot of converters in your mappings, you will have to lazy load them each test. You won't load all of them (hopefully), but if you do then for each test you will have to redo it each time. How much time this will take and if this is a priority or not I don't know. |
|
I don't see a difference in our tests. And anyway, we're not going to optimize for that now :) |
|
@jbogard Can we merge this? I want to direct people to the MyGet build. |
|
@lbargaoanu I left some questions, as this is a breaking change. |
|
Well, yes, but it's simple enough to pass the mappers. We can't keep a static property... |
|
Well, we could with ThreadStatic, but I suppose that's out of the question :) |
|
In that case, if it's going to break, I'd rather just break it in a consistent way. This is a bit of a one-step-forwards-two-steps-back approach. I'll amend your PR... |
Exposing mapper list through the overall mapper configuration
|
I like it. @TylerCarlson1 the only downside still is that you can't access the mapper registry from a profile, that would be nice. But let's look at that in a follow-up PR. @lbargaoanu is this good to go? |
|
Yes. |
| { | ||
| private static readonly IObjectMapper[] _initialMappers = | ||
| public static IObjectMapper[] Mappers() => new IObjectMapper[] | ||
| { |
There was a problem hiding this comment.
This is a weird method. I wonder if we could just expose the property still, but make it very explicit of an IReadOnlyCollection? What would be the tradeoff?
There was a problem hiding this comment.
This is also a breaking change to the API. Is there a way to preserve the property but make this thread-safe?
There was a problem hiding this comment.
No, didn't see them :)
It doesn't help to make it ReadOnlyCollection, it's still a breaking change. The only way I can think of to keep it is through ThreadStatic. But that doesn't really make sense if it's exposed through config. It's breaking, but minor, few people use mappers I would say.
There was a problem hiding this comment.
Ha! I was trying out the Review feature in GitHub. Honestly don't understand the point of it.
jbogard
changed the title
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
Closes #1847.