Use PolyShim for polyfilling types and members on older targets

[Tyrrrz]

No description provided.

[abergs]

I don't hold any opinion on this. Maybe @justindbaur?

ps.
If Polyshim wasn't built by Oleksii I would question if we really want to add extra dependencies (that may or may not break in the future) -- but since we know the author I don't mind.

[Tyrrrz]

Readme of the PolyShim project should possibly remove any references to the Ukraine-Russia war, given it can impact any relationships with clients.

Please explain how it would "impact any relationships with clients"

[jrmccannon]

LGTM

[justindbaur]

I'm pretty neutral on this. While I like that it's less code and what is better than a PR that deletes code. If it works the way I suspect, we will have more final code and I'd sorta rather only polyfill the things we actually need.

I suspect you built this to do a targets import of various .cs files so this doesn't even show up as a dependency to downstream consumers yeah? If so, I do like that and I think it pushes me more into the yes column.

[Tyrrrz]

I'm pretty neutral on this. While I like that it's less code and what is better than a PR that deletes code. If it works the way I suspect, we will have more final code and I'd sorta rather only polyfill the things we actually need.

Yeah the final assembly (only for older targets) will include slightly more members but they're all internal. It won't have any noticeable impact on size or performance though. Additionally, tree shaking should negate the downside completely.

I suspect you built this to do a targets import of various .cs files so this doesn't even show up as a dependency to downstream consumers yeah? If so, I do like that and I think it pushes me more into the yes column.

Indeed. See here: https://github.com/Tyrrrz/PolyShim/blob/aa5f4e7a6d4594d2f0256dc03d7525b0d9338000/PolyShim/PolyShim.csproj#L34-L35

The polyfills themselves are only included on targets that require them, for example:

https://github.com/Tyrrrz/PolyShim/blob/aa5f4e7a6d4594d2f0256dc03d7525b0d9338000/PolyShim/NetCore21/StreamReader.cs#L1

Note that the impact of removing just one polyfill (as is the case here) isn't that significant, but it typically doesn't stop at one polyfill 😅

I'm also okay with us using a different polyfill library, such as https://github.com/SimonCropp/Polyfill or https://github.com/Sergio0694/PolySharp. The benefit of PolyShim is that I maintain it, meaning that I can expand the polyfill coverage as needed by our projects.

[justindbaur]

Good point, lets do it then. Just don't start using ValueTuple in the public API surface!

[jonashendrickx]

Readme of the PolyShim project should possibly remove any references to the Ukraine-Russia war, given it can impact any relationships with clients.

Please explain how it would "impact any relationships with clients"

We might have pro-Russian clients, or simply clients that do not want to get involved in the Russia-Ukraine war. We should as a company remain neutral. It also risks Bitwarden products turning into protestware.

[Tyrrrz]

We might have pro-Russian clients

We better not, otherwise I wouldn't be working here

[dasien]

“After reviewing the terms of use for this library, we are closing this PR. To be clear, this is not done out of consideration for any clients’ views. Bitwarden prohibits the sale of our products to companies and countries on the US Sanction list. Bitwarden does not include libraries which have terms of use requiring our clients to take positions on social or political issues.”