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

Align runtime requirements with EF packages #3

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

TobbenTM
Copy link
Contributor

@TobbenTM TobbenTM commented Aug 17, 2023

An issue we found using this package is that as soon as you upgrade to EF 7.0 packages, but still running on .Net 6, it fails since it's referencing the wrong EF assemblies.

Specifically, we're seeing issues like this:

System.MissingMethodException: Method not found: 'System.Collections.Generic.IList`1<Microsoft.EntityFrameworkCore.Metadata.Conventions.IModelFinalizingConvention> Microsoft.EntityFrameworkCore.Metadata.Conventions.ConventionSet.get_ModelFinalizingConventions()'.
   at XO.EntityFrameworkCore.NpgsqlJsonSerializerOptions.JsonSerializerOptionsConventionSetPlugin.ModifyConventions(ConventionSet conventionSet)

To align this package with how EF packages are published (all compatible with .Net 6, even 7.0 packages), this PR removes the runtime conditional switch for dependencies.
In this move, support for EF 6 is also dropped, but I'd argue that anyone needing EF 6 compatibility could use a earlier version of this package, just like how they consume EF itself.

Would love to hear your thoughts on this, because we've found this package quite helpful while we await EF 8.0!

@wjrogers
Copy link
Member

Sounds reasonable to me. I wanted to make the library compatible with both versions 6 and 7 of EF Core, but I suppose publishing different major versions would have been a better way of doing that. Thanks for contributing!

@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #3 (80d5386) into main (1f956de) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #3   +/-   ##
=======================================
  Coverage   94.20%   94.20%           
=======================================
  Files          10       10           
  Lines         138      138           
=======================================
  Hits          130      130           
  Misses          8        8           
Components Coverage Δ
XO.EntityFrameworkCore.NpgsqlJsonSerializerOptions 94.20% <ø> (ø)

@wjrogers wjrogers merged commit 098d15c into xo-energy:main Aug 17, 2023
2 of 3 checks passed
@TobbenTM
Copy link
Contributor Author

Yeah, having matching versions, maintaining different packages for 6.0 and 7.0 is probably "the most correct" approach, but also takes effort.
Anyways, with the latest 2.0.4 version you published with these changes, our app is happy again, able to do nice ser/de!
Thanks for the quick response here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants