Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

real DI scoping #74

Merged
merged 2 commits into from
May 7, 2022
Merged

real DI scoping #74

merged 2 commits into from
May 7, 2022

Conversation

shlomiassaf
Copy link
Contributor

@shlomiassaf shlomiassaf commented Dec 15, 2021

The current implementation is missing some of the goals of DI.

  • It will create a new service container on every scenario (which might be slow)
    • It will build the service provider on every scenario (which might be slow)
  • The above is true for Features as well, i.e. Injections from Features will not be the same as in Scenario!!!
  • It does not provide singleton option in the DI, as the DI is built every time
  • It does not dispose the service providers, it's not possible since it's always the root provider
  • It does not provide scope resolution. I.E Feature based scoping vs Scenario based scoping
  • Because a new container was created on every scenario, a new instance of the binding class was created, not allowing property share via instance.

This PR fixes scoping for Scenario and add support for Feature scoping.

With Scenario:

  • The service container is now created and built ONCE for the entire run
  • Singleton are now possible for the entire run.
  • Each Scenario is now a real IServiceScope which is also disposed once the Scenario is done

With Feature:

  • The service container is now created and built ONCE for the entire run
  • Singleton are now possible for the entire run.
  • Each Feature is a IServiceScope which is also disposed once the Feature is done
  • All Scenarios are injected using the same scope, i.e. they share injections
  • Feature hooks can now get injections shared with the injection of the Scenario

To use Feature scoping

[ScenarioDependencies(ScopeLevel = ScopeLevelType.Feature)]

The default is Scenario to ensure backward compatibility

@shlomiassaf
Copy link
Contributor Author

@mbhoek I know it might look overwhelming, I hope you invest the time to get into it, because this is a big improvment.

@mbhoek
Copy link
Member

mbhoek commented Dec 22, 2021

@shlomiassaf Thank you for your contribution! I'm currently busy working on another project, but will have time to review your PR during the upcoming holidays. Sorry for the delay!

@mbhoek
Copy link
Member

mbhoek commented Mar 12, 2022

Hello @shlomiassaf, I finally found some time to review your PR and I think it's excellent work. I'm going to include in the next big update of this plugin, which will also include fixes for #75. I'd like to combine those fixes so I want to merge your work into a different branch next instead of main. I'm moving forward with this assuming you are okay with this. Thanks again!

@mbhoek mbhoek changed the base branch from main to next March 12, 2022 14:45
@shlomiassaf
Copy link
Contributor Author

I appreciate your update.
Looking forward for it, we're working with a side branch.

@mbhoek
Copy link
Member

mbhoek commented Mar 13, 2022

I just realized if I merge your work into a separate branch, and then later merge it into main, you would not get the proper attribution/credits in the 'Contributors' section on GitHub. So I'm going to have to rethink this.

@shlomiassaf
Copy link
Contributor Author

shlomiassaf commented Mar 13, 2022 via email

@mbhoek
Copy link
Member

mbhoek commented Mar 15, 2022

I think contributions to open source projects should be properly contributed, I really appreciate you spending your valuable time on this. After giving it some thought the solution was pretty simple; enabling pre-release versions of the package allows me to complete PRs without publishing a release version.

Switching your PR back to main.

@mbhoek mbhoek changed the base branch from next to main March 15, 2022 20:39
Copy link
Member

@mbhoek mbhoek left a comment

Choose a reason for hiding this comment

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

Looks good to me! :shipit:

@mbhoek mbhoek merged commit 1258fb0 into solidtoken:main May 7, 2022
@mbhoek mbhoek linked an issue May 7, 2022 that may be closed by this pull request
@mbhoek mbhoek linked an issue May 7, 2022 that may be closed by this pull request
@mbhoek
Copy link
Member

mbhoek commented May 7, 2022

Thanks again for your excellent work @shlomiassaf -- your contribution is now part of the latest preview release.

@mbhoek mbhoek removed their assignment May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Scoping Context Injection to Scenario Execution Lifetimes
2 participants