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

Merge immutable collections formatters into main library #1029

Merged
merged 3 commits into from
Sep 9, 2020

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented Sep 9, 2020

This removes the need for a separate assembly and package to support immutable collection types being serialized/deserialized. It also removes the need to manually link in the immutable collection resolver as it is now included in the standard resolver.

This requires that we ship at least one more version of the MessagePack.ImmutableCollection package in order to deliver the type forwarders. After that, we could delete the project from the repo and never ship it again. The function of the type forwarders is to that if someone compiled an assembly that references MessagePack.ImmutableCollection.dll, the CLR will demand that that dll be present at runtime with those types. The type forwarders allow the CLR to recognize that those types moved from one DLL to another and keep functioning. The DLL is only necessary at runtime when other dll's were compiled against it -- otherwise it can be omitted from an application.

Closes #606

@AArnott AArnott added this to the v2.2 milestone Sep 9, 2020
@AArnott AArnott requested a review from neuecc September 9, 2020 01:29
@AArnott AArnott self-assigned this Sep 9, 2020
I leave behind type forwarders so that folks who compiled against the old location for these public types will still run successfully with no type load exception.
Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

ImmutableCollection should not add under Unity.

@AArnott
Copy link
Collaborator Author

AArnott commented Sep 9, 2020

Thanks for reviewing, @neuecc. Are you saying the formatters should not be included in the Unity version? If we exclude it, how do you see Unity customers adding immutable collection support if they want it? Or are immutable collections broken under unity anyways?

@neuecc
Copy link
Member

neuecc commented Sep 9, 2020

@AArnott
We have two choices.

  • Include System.Collections.Immutable.dll in Unity's package(your PR have xcopy, so currently choosed this)
  • Exclude Formatter/Resolver of ImmutableCollection

Until now, there was no way to add a formatter for the ImmutableCollection (other than by copy-paste from the source code)

As far as I'm concerned, I think it's fine to exclude it completely.
First of all, as I've been repeating all along, it's very important to minimize dependencies.
Especially in Unity, where there is no way to resolve assembly versions, adding unnecessary dlls should be avoided.
And Immutable Collections are completely unattractive and undemanding in Unity (lots of allocations are not preferred in games).
If you really want to add it, you can cover it with copy and paste (you can add the .unitypackage independently, but there's little demand for it)

@AArnott
Copy link
Collaborator Author

AArnott commented Sep 9, 2020

OK. Thanks for the clarification. I'll omit these from the unity package.

@neuecc
Copy link
Member

neuecc commented Sep 9, 2020

Thanks, okay to merge.

@AArnott AArnott merged commit 2bb600e into MessagePack-CSharp:v2.2 Sep 9, 2020
@AArnott AArnott deleted the mergeImmutableCollections branch September 9, 2020 20:37
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.

None yet

2 participants