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

Unity 5.0.0-beta targeting netstandard 1.0 #1175

Closed
dersia opened this issue Sep 26, 2017 · 50 comments
Closed

Unity 5.0.0-beta targeting netstandard 1.0 #1175

dersia opened this issue Sep 26, 2017 · 50 comments

Comments

@dersia
Copy link

dersia commented Sep 26, 2017

Unity has a new Maintainer and they started refactoring to target netstandard 1.0.

Since PrismLibrary is also moving towards netstandard1.0 it makes sense to reference that preview version of Unity 5.
The new Maintainer also updated CommonServiceLocator to 1.4. Unfortunately the new CommonServiceLocator targets netstandard 2.0 (which makes sence since there was no ServiceLocator API avl. in netdtandard prior to 2.0).

I updated the references for Prism.Unity.Forms to Unity 5.0 and created a PR: #1174

Unity 5 comes with changes in the namespace and this could be a possible breaking change, although it would be just about updating namespace like using Microsoft.Practices.Unity to using Unity

This works so far fine for Prism.Unity.Forms.

I also started changes on Prism.Unity.Wpf and Prism.Unity.Windows. I didn't want them in the same commit as with Prism.Unity.Forms, because other than Prism.Unity.Forms, .Wpf and .Windows rely on CommonServiceLocator as a first class dependency.
Updating Wpf is no problem so far, since CommonServiceLocator targets also net40, net45 and net47.
.Windows on the other hand unfortunately does not support netstandard 2.0 yet and there is no target on CommonServiceLocator for uap10.0.10240. But updating Unity to 5 works fine, even if we stay with CommonServiceLocator 1.3

I don't like the Idea of using different versions of the same dependency throughout prism, but in the end it comes down to what the community and the prism core team things of this.

@bartlannoeye
Copy link
Contributor

UWP does support .netstandard 2.0 if you work on the insider previews / latest SDK preview. However we didn't upgrade our solution yet. So we can update that later.

@dersia
Copy link
Author

dersia commented Sep 26, 2017

@bartlannoeye yes, but this also means, that Prism.Windows would only support apps targeting FCU and higher, not to say that it then also would just support Feature2 on Windows 10 Mobile.

This is why I would rather stay with the old CommonServiceLocator. I also looked into the possibility to submit a PR for CommonServiceLocator by simple just targeting also uap10.0, but there seems to be an issue with MSBuild right now: dotnet/sdk#1556

As soon as this is done, we could revisit UWP

@bartlannoeye
Copy link
Contributor

We'll always have Prism.Windows v 6.3 (or maybe 6.4 with some fixes) if people want to target older versions. I see .netstandard support for v 7.0, which would already have other breaking changes. If using v7 means being limited to FCU, so be it. That's something we'll decide on when the time is there.

@dersia
Copy link
Author

dersia commented Sep 26, 2017

Should I then create an PR with an updated version of Prism.Windows targeting FCU?

@bartlannoeye
Copy link
Contributor

I'm waiting on 2 in-person meetings next month before making the decision with the rest of the Prism team on how breaking 7.0 will be and which version we'll target. So for now, don't put effort into it yet.

@dersia
Copy link
Author

dersia commented Oct 3, 2017

@bartlannoeye @brianlagunas
Any updates on this? There is also a new stable version of Xamarin.Form 2.4 released and a version 2.0 of the CommonServiceLocator. I'd really like to update all my packages to netstandard2.0.

I think we can't avoid any breaking changes.

@dansiegel
Copy link
Member

I feel like I'm missing something... but if Unity 5 doesn't have a dependency on CommonServiceLocator why do we care about it?

@dersia
Copy link
Author

dersia commented Oct 3, 2017

We are using CommonServiceLocator in the base projects lik Prism.Wpf, Prism.Windows. And then again all other containers autofac, unity, etc. Are using it too.

@dersia
Copy link
Author

dersia commented Oct 3, 2017

Just as an update, so I updated the win10 projects to FCU and updated all usages of CSL and unity.

There are some issues.
For UWP to work correctly with netstandard2.0 I had to update the uwp-runtime packages to version >=6.0.0. Unfortunately those packes aren't yet on nuget, I grabbed them from the dotnet myget feed.

For WPF everything works except the DryIoc packages. DryIoc has a hard dependency on CSL >=1.3.0 & <2.0.0
Unless DryIoc updates (I created a ticket on their bitbucket tracker) we either have to wait or drop DryIoc. Since there is only DryIoc support for wpf, but not for forms or win10, I'd vote for this option. But then again, I never used DryIoc and I don't know how big the userbase is.
DryIoc for Forms is not using the DryIoc.CommonServiceLocator and therefor is not affected by this issue at all.

@brianlagunas
Copy link
Member

As of now, there are tons of issues with .NET Standard across many platforms. Heck, we can't even get our builds working properly with the updated dependencies. We will be moving slow on this issue.

@ENikS
Copy link

ENikS commented Oct 3, 2017

The package will have .NET 4.0, 4.5, 4.7, and Standard 1.0 so you don't have to go with the lowest denominator. I would wait with integration though because I am planning some work on the container's engine before releasing it.

CommonServiceLocator is no longer part of the core container and assembly no longer references it. You could have your own implementation of the adapter and could use either 1.0.3 or 2.0.0 or both if you want.

@ENikS
Copy link

ENikS commented Oct 12, 2017

I released beta2. I think it is stable enough for you to start integration.
Release notes

It would be nice if you could give some early feedback before I move to 5.0.0 release

@dersia According to MS blog UWP is now supports Netstandard 2.0 API. Please let me know if you can reference Unity from the latest UWP.

@dansiegel
Copy link
Member

@ENikS was noticing that our tests break when updating to v5 unless I change the test project to net47

@ENikS
Copy link

ENikS commented Oct 25, 2017

@dansiegel Is there a question? You do understand that you do not give me any info to answer intelligently?

  • What tests
  • What errors
  • Why it works in net 4.7

@dansiegel
Copy link
Member

@ENikS yes sorry, thought maybe you might be aware of something off the top of your head. Right now I was updating Prism.Forms.Unity and the corresponding test project is a netcoreapp1.1. The error on the tests is:

Message: System.IO.FileNotFoundException : 
Could not load file or assembly 'Unity.Container, Version=5.0.0.0, Culture=neutral, 
PublicKeyToken=489b6accfaf20ef0'. The system cannot find the file specified.

Since the tests should be using the netstandard version the same as an iOS or Android project this leaves me a little concerned. You can see my branch here: https://github.com/dansiegel/Prism/tree/unity

@ENikS
Copy link

ENikS commented Oct 26, 2017

Sent you PR with the fix

@brianlagunas
Copy link
Member

@ENikS I would suggest changing the Unity package to remove all assemblies and just have dependencies on your new Unity.Container and Unity.Abstractions packages.

Currently, this is a major issue for anyone look to upgrade. If you are recommending those two other pages going forward, this is what I would do to make the move seamless.

@ENikS
Copy link

ENikS commented Oct 26, 2017

My only concern with this approach is organizations with high security requirements. Their onboarding process for packages requires a lot of hustle and verifications. Including all dlls into package eliminates most of it. Here, in NY these are majority.

I am not opposed to the idea of referencing packages I just don't know what is best. Can you explain your reasons why you think it is not a good idea to packaging DLLs as opposed to referencing them?

@brianlagunas
Copy link
Member

You see the issue we ran into here, and this is a simple case. Now, if we switch to using the Unity.Container package, every install of Prism for XF will have to uninstall the previous Unity package. This is going to be a painful upgrade.

@dansiegel
Copy link
Member

@ENikS I agree it would be far easier to just have a single package dependency. But ultimately if a package consumer cannot reference the Unity Package and have it work, it becomes useless. I also tried 5.0.2 and had the same resulting error. I'm all for the way you have it now, if you can make it work. Ultimately between having to update namespaces and then having to deal with different packages because the dll's aren't actually getting referenced correctly, as Brian said that would be very painful for a lot of developers.

@ENikS
Copy link

ENikS commented Oct 26, 2017

If you target individual packages as I recommended in PR you should be just fine.
I am thinking about companies like Wells Fargo or Morgan Stanley, they all use Unity and for them reference approach might not work.
Again, you might be right and solution you are proposing might be better. I just need more research on the subject to make a decision.

@brianlagunas
Copy link
Member

FYI, Wells Fargo and Morgan Stanley use Prism too. So they would have this painful upgrade experience as well.

@ENikS
Copy link

ENikS commented Oct 26, 2017

Would you do a little experiment for me? Grab nuspec file in Unity repo and change it to references. See if it solves your issues?
I have a suspicion it is MSBuild that causing the problem. It does not know what DLLs to copy to the test bin directory and changing it to REFs will not make a difference.

@ENikS
Copy link

ENikS commented Oct 26, 2017

@brianlagunas You don't have to tell me, I have intimate knowledge of the process. Worked for them both ;).

@ENikS
Copy link

ENikS commented Oct 26, 2017

I have a feeling this issue with referencing DLLs from composite libraries is the reason Microsoft went the same route with .NET itself. Now every DLL is separate package.

@brianlagunas
Copy link
Member

There are a lot of namespace changes that will make upgrading/using unit a real pain. For example, the ContainerControlledLifetimeManager is now under the Unity.Lifetime namespace. This means I have to add two namespaces just to register a singleton. Things like this will make using unity somewhat of a pain.

@ENikS
Copy link

ENikS commented Nov 4, 2017

If you have an idea how to make it better please let me know.
I put it there based on my, somewhat limited, best judgment but codebase is huge so it is a bit hard for one person to think about every scenario and type.

... or better yet, send a PR

@brianlagunas
Copy link
Member

Well, honestly, you would have probably been better off leaving the nested namespaces how they were. Removing the Microsoft from them is fine, but you moved a ton of objects around and basically make upgrading undesirable. The PR would be to put everything back where it was and just remove Microsoft from the namespace. Unity has so many installs, making such a broad changes to objects locations and namespaces is really going to be a painful experience.

But, you have already move them so the best you can do is try to improve the developer experience. Maybe creating a new API that allows using the basics of the container (such as registering) easy and not require multiple namespaces for the most basic function.

container.RegitserType<IType, Type>().AsSingleton();

That would essentially hide the knowledge and required namespace of the ContainerControlledLifetimeManager from the developer.

I fI didn't have my own very large project to manage, I would be submitting PR's :)

@dansiegel dansiegel mentioned this issue Nov 4, 2017
@ENikS
Copy link

ENikS commented Nov 4, 2017

I will add RegisterSingleton<,>(name) in next patch release.

Unity has so many installs, making such a broad changes to objects locations and namespaces is really going to be a painful experience

Most of us use ReSharper for that purpose, it makes refactoring no brainer (Visual Studio 2017 does it too). Out of about 15K new downloads of v5.1.2 you are the first to complain about it.
My offer still stands, if you/Prism team think experience could be improved by moving types from namespace to namespace I am willing to consider.

Having flat Unity namespace (this is what you advocating) effectively prevents extending any of these types without breaking compatibility with previous version. It makes LTS maintenance for packages impossible and defeats the purpose of having Abstraction assembly.

@brianlagunas
Copy link
Member

Most of us use ReSharper...

You can't rely on everyone having Resharper. I don't use it because it's a resource hog and slows VS down to a crawl when loading solutions, and other reasons.

you are the first to complain about it

I guess since I'm the first to complain there is no merit in my feedback.

Having flat Unity namespace (this is what you advocating) effectively

This is not what I am advocating. I am advocating of keeping your API easy to use by reducing the number of required namespaces to use the most basic features. For example to simply register a type as a singleton, I am now required to have knowledge of two namespaces just to do a basic feature of the container. This is not a good developer experience.

It's great you are moving Unity forward. Many of us appreciate it, especially me. I have been using Unity since 2008. It's the only container I have used. I wish you nothing but luck as Prism has a heavy investment in Unity.

@ENikS
Copy link

ENikS commented Nov 4, 2017

I guess we are on the same page than.

@brianlagunas
Copy link
Member

A quick update. Prism will have to postpone supporting v5 until a critical Unity bug is fixed. It was reported here: unitycontainer/abstractions#17

I will update this thread when an update with a fix is available.

@ENikS
Copy link

ENikS commented Nov 7, 2017

The Mock types need to be updated to pass these tests. IUnityContainer has changed.

@brianlagunas
Copy link
Member

Done

@ENikS
Copy link

ENikS commented Nov 8, 2017

I see you are officially on v5. Congrats!

@ENikS
Copy link

ENikS commented Nov 8, 2017

I though I would add this to resolve discussion about composite vs reference packages. I had this issue filed with another project for exactly the same problem. They followed reference strategy and made whole package completely unusable behind firewall.

So, long story short: Unity will be all inclusive package with all core DLLs packed into it.

@Shashanka77
Copy link

@brianlagunas Can you please give a rough estimate on when the new Prism.Unity will be available? I am supporting latest unity library.

@brianlagunas
Copy link
Member

I have no time frame. For now, you can use our CI builds on MyGet.

@ENikS
Copy link

ENikS commented Nov 13, 2017

Added lineup of extension methods for registering Singletons so you don't have to include additional namespaces

@brianlagunas
Copy link
Member

Good to know, thanks.

@Shashanka77
Copy link

I know this is may not be the platform for this question. We had recently migrated from Unity to Autofac Prism not just because Prism's unity with latest isn't ready yet but mainly because as we are consuming .NETStandard 2.0 libraries in our WPF Prism application. We were able seamlessly convert into AutoFac prism without much hassle. But problem we are facing now is we are getting "Could not load System.Runtime.dll" error upon installing via ClickOnce. Please NOTE we also migrated to .NET 4.7.1 in order better solve this issue. Any insights in this respect will be greatly appreciated. Thank you

@1valdis
Copy link

1valdis commented Dec 13, 2017

So how long it's going to take to move Prism to Unity 5? I've tried to use Unity 5.3.2 and I can't even override CreateShell in my bootstrapper because there's no IUnityContainer which it wants.

@dansiegel
Copy link
Member

@1valdis Prism already has updated to Unity 5. Please update your project to the Prism 7 preview for Unity 5 support.

@1valdis
Copy link

1valdis commented Dec 13, 2017

@dansiegel Wow, didn't looked for pre-release version. Thanks!

@ENikS
Copy link

ENikS commented Jan 4, 2018

@brianlagunas @dansiegel Any estimate on release date?
Here, at MS we are doing major upgrade to the framework but we couldn't upgrade Unity without Prism.

@brianlagunas
Copy link
Member

Prism for XF v7 will release next week. You can use the previews now if you like.

@ENikS
Copy link

ENikS commented Jan 4, 2018

Thank you, next week works for us.

@ENikS
Copy link

ENikS commented Jan 25, 2018

@brianlagunas @dansiegel Hate to be "Are we there yet?" person but have no choice.

The company's framework team moved on with preview version and now stuck with it unable to release. Is there anything we can help you with to expedite the release? Something we could send you a PR for?

@brianlagunas
Copy link
Member

Prism for XF v7 has been released. If you are talking about WPF, then there is no release date for v7. We don't have released dates as we are not a company with paid employees or paying customers :). We simply release when we have time to finish what we wanted to accomplish for a version.

We have a few tasks we still need to complete before releasing which may take awhile. Unfortunately, the main task that of aligning modularity is not something that anyone can help with. I will be doing that. Though, if you want to attempt a new IDialogService based on the discussions in #864, you could start there.

If you don't want to use the preview, even though we aren't changing the bootstrapper so everything will work just fine, you can always download the 6.3 source from the Tag and then upgrade Unity and use your own build for now. That will unblock you.

@lock
Copy link

lock bot commented Jan 29, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants