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

Make using of ImTools concurrently with DryIoc #30

Closed
detoxhby opened this issue Feb 20, 2020 · 13 comments
Closed

Make using of ImTools concurrently with DryIoc #30

detoxhby opened this issue Feb 20, 2020 · 13 comments

Comments

@detoxhby
Copy link

Current simpton of trying to use ImTool with other projects that encapsulates this library is impossible.

It may sounds strange, but makes sense when DryIoc and ImTools itself is on different upgrade path in a project. The container will always be in lower version due to safety and compatibility, while in other parts of the project the new functionality of the ImTools could be benefical.

I would suggest using compile macro usage to differentiate the base namespace between embedded and normal usage of the library, thus making it easy for everyone.

Any response is appreciated. Thanks!

@dadhi
Copy link
Owner

dadhi commented Feb 20, 2020

Thanks for the open issue.
It may be a real PITA actually.

What is your setup?

  • DryIoc source or dll package?
  • ImTools source or dll package?
  • Are they referenced from the different package
  • Did you know about DryIoc.Internal package?

@detoxhby
Copy link
Author

detoxhby commented Feb 20, 2020

DryIoc source or dll package?
ImTools source or dll package?

DLL for both packages.

Are they referenced from the different package

This is the problem: cannot reference it as the compiler gets confused as both assemblies contains the same namespace.

Did you know about DryIoc.Internal package?

I did not, but checking out the nuspec file it will still include all the external lib sources and would cause the conflict.
Please correct me if I am mistaken.


To be complete, the same happens with FastExpressionCompiler as well. In that case, we had to change the namespace locally to be able to use it separately from DryIoc. Would be the solution for ImTools as well, but I don't wanna go this way because source inclusion...

  • adds unnecessary extra time for compilation, and
  • confuses code quality tools (like SonarQube) that will throw numerous warnings, and would impair the code duplication, reliability and other indexes, thus altering the real state of the project's source code ( this is what happened with FEC, not good )

@dadhi
Copy link
Owner

dadhi commented Feb 20, 2020

DryIoc.Internal makes all public types internal including the types from FEC and ImTools.

Thinking about this Today and I see two ideal choices:

  1. Make FEC and ImTools sources in DryIoc into the implementation detail - internalize the public types, optionally strip the unused code.
    Pros:
  • No confusion with standalone FEC and IT
  • Still no external deps to manage
  • Easy to tune for DryIoc needs
  • Less code and dll size if the stripping is in place

Cons:

  • Breaking DryIoc major version
  • More management from my side - harder to incorp the fixes (though it maybe less fixes because of stripped code)
  1. Make FEC and IT the explicit dll deps.
    Pros:
  • Familiar dll story
  • Easy to incorp new fixes
  • Non-breaking?

Cons:

  • Harder to tune for DryIoc
  • Dll deps hell
  • Not sure how to proceed with the source packages - breakes here?

@detoxhby What are your thoughts?

@detoxhby
Copy link
Author

detoxhby commented Feb 20, 2020

Thanks for the ideas, I will think about the options how to solves this in a balanced manner!


In the meantime, what about my original idea?

In the external libs (FEC, IT), introducing a preprocessor #if directive based macro that would include .Embedded after every single namespace. Could be able to manage the refactor with regex-based replace.

For example:

namespace ImTools
#if IMTOOLS_EMBEDDED
    .Embedded
#endif
{
   ...

Pros:

  • no maintenance costs: would be able to include the unmodified source into DryIoc
  • insignificant refactor needs:
    • relatively easy find in path + replace using ImTools by using ImTools.Embedded
    • set IMTOOLS_EMBEDDED symbol for compilation
  • no change in distribution options: people still be able to include the sources directly into their code without any change or the need to set a precompile symbol (becase the default behavior would produce the corrrect namespace)
  • ⚠️ no external changes: DryIoc users would've never used the extra libs directly from DryIoc, so would not break legal code

Cons:

  • ❗️ external changes: would unintentionally break working code just by the fact the mistaken references are would no longer be available

No easy way to deal with the con other than jumping at least a minor version.
I know the semver would make it necessary to jump a major, but I don't think it is a direct API change, it is just an incorporating dependency change which is outside the libraries' functionality.
Another option is to introduce a new NuGet package (like DryIoc.Embed) which would behave this way without making the useful types internal.

@dadhi
Copy link
Owner

dadhi commented Feb 20, 2020

You mean IMTOOLS_EMBEDDED will be set in DryIoc packages, right?

Then I think it is a good thing to go when thinking in the smaller steps. Thanks for the idea.
You can submit a PR if you want.
I'll probably will risk to go with the v4.2 - I've planned a more cleanup for the v5... lemmethink

@detoxhby
Copy link
Author

You mean IMTOOLS_EMBEDDED will be set in DryIoc packages, right?

Yes!

You can submit a PR if you want.

I have some serious deadlines currently for the next few weeks, but after that, I would gladly help with this work throughout all the projects (FEC, IT, DryIoc).

How do you want to handle cross-project refences to coordinate this change? (FEC+IT first with minor version change and release THEN DryIoc with these new versions?)

@dadhi
Copy link
Owner

dadhi commented Feb 21, 2020

I have some serious deadlines currently for the next few weeks

Maybe I have time to look next week.
I am thinking about minor versions for FEC and IT, and probably major for DryIoc. I'd like to have an oportunity and mark some things Obsolete in DI. Given that prev release of DI 4.1 was stretching a line of minor version - now is time for fair and square major change.

@detoxhby
Copy link
Author

✔️ Alright, this is not a rush, take the time necessary for a correct major version change!

Feel free to contact me if you need more information or a second-eye with ideas or realization details.

@abelbraaksma
Copy link

Maybe a crazy thought, but why not make ImTools independent, and simply have Dryloc reference ImTools? That way, if projects reference both, there won't be any problem anymore, as there'll be only one shared library for the ImTools classes.

@dadhi
Copy link
Owner

dadhi commented Feb 23, 2020

@abelbraaksma

Maybe a crazy thought, but why not make ImTools independent, and simply have Dryloc reference ImTools?

I've answered to this with the second choice in the discussion above.
Mainly I don't wont to get the same thing as with Json.NET dep.
Plus I still want a freedom in DryIoc to use a specially tuned versions of deps. It is just an impl detail - let's keep it that way.

@detoxhby
I'm thinking about simplier variant to your idea. Just change the namespaces in DryIoc to DryIoc.ImTools and DryIoc.FastExpressionCompiler respectevely.

@abelbraaksma
Copy link

I've answered to this with the second choice in the discussion above.

Apologies, I thought I read the whole discussion, must've missed it ;)

It is just an impl detail - let's keep it that way.

Fair enough 👍

@detoxhby
Copy link
Author

I'm thinking about simplier variant to your idea. Just change the namespaces in DryIoc to DryIoc.ImTools and DryIoc.FastExpressionCompiler respectevely.

That's fine too, but a bit error more prone. If you copy over new code and forget about the namespace, you could easily make a VS quick action include mistake somewhere in DryIoc's source code unintentionally (there is a chance; but "fixing" the sources would discard this case entirely).

ℹ️ to reason about current single-file situation: I think it's fine to go with the easy way and just rewrite the namespace in the target project
(would not recommend though if IM or FEC would contains more than a few namespaces and/or files)

@dadhi
Copy link
Owner

dadhi commented Mar 21, 2022

implemented in DryIoc v5

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

No branches or pull requests

3 participants