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

Refactor resolver #1321

Closed
notaphplover opened this issue Apr 30, 2021 · 8 comments
Closed

Refactor resolver #1321

notaphplover opened this issue Apr 30, 2021 · 8 comments
Assignees

Comments

@notaphplover
Copy link
Member

As commented at #1132 (comment), Resolver can be refactored.

  • A resolveBinding function could be extracted.
  • result is guaranteed to be undefined | T | Promise if !(targetIsAnArray && targetParentIsNotAnArray)
  • resolveInstance should be passed as generic paramenter:
...
        } else if (binding.type === BindingTypeEnum.Instance && binding.implementationType !== null) {
            result = resolveInstance<T>(
                binding,
                binding.implementationType,
                childRequests,
                _resolveRequest<T>(requestScope)
            );
        } else {
...
@notaphplover
Copy link
Member Author

@tonyhallett This is the issue, do you agree to all those statements? If that's the case just answer this message and I'll submit a PR

@tonyhallett
Copy link
Contributor

Yes !

The T in `Binding' is TActivated, the resolved value of T.
add image here
The T in the "get" methods applies equally to resolve - if you type correctly you get it back from the binding ( or cache )
image

@tonyhallett
Copy link
Contributor

We also need to change the nonsense T's to any

public factory: interfaces.FactoryCreator<T> | null;
to match the interface.

@tonyhallett
Copy link
Contributor

Perhaps we should change Binding<T> to Binding<TActivated> to be clear.

@tonyhallett tonyhallett self-assigned this May 1, 2021
@tonyhallett
Copy link
Contributor

@notaphplover Taking this one

@notaphplover
Copy link
Member Author

@notaphplover Taking this one

I already made the PR sorry. There is a test which fails on node14 es6 build, I'll be missing for some hours so if you want could you see what's happening?

@tonyhallett
Copy link
Contributor

I will have a look

@notaphplover
Copy link
Member Author

Closing as #1323 is merged

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

No branches or pull requests

2 participants