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

inject-lazy implementation. #8

Open
wants to merge 4 commits into
base: inject-lazy
Choose a base branch
from

Conversation

tripodsgames
Copy link

First of all, I loved your project.
I would like to leave a contribution here for an alternative to asynchronous injection, it uses Proxies and intercepts access to the object.

HELP WANTED.

TODO

  • Fix Maximum call stack size exceeded.
  • Improve Tests.
  • Create docs about the feature.

@zheksoon
Copy link
Owner

Wow, that's huge!!!

Thanks a lot for the contribution, I will merge it in a separate branch and add some refactoring (already have it done)

@zheksoon zheksoon marked this pull request as ready for review April 18, 2024 15:38
zheksoon added a commit that referenced this pull request Apr 18, 2024
@zheksoon
Copy link
Owner

Ok, so the main changes of the refactoring:

  • Now you don't have to wrap the class you are injecting into arrow function. You can just use usual class name, like this:

    constructor(public instanceB = injectLazy(CircularDependencyB)) {}

    If you see a usecase for the arrow function, let me know - it should be easy to add

  • New error has been added, now it's just generic, with a lengthy text:
    Transient scope for class ${cls.name} was resolved too many times in one tick. This is likely an infinite loop.
    It signifies that you have resolved one class too many times during one microtask, so this means you are likely in an infinite loop (he-he, the same words)

Otherwise, that's great contribution, and I'm happy to see the real people use it!

@zheksoon zheksoon changed the base branch from main to inject-lazy April 18, 2024 16:08
@tripodsgames
Copy link
Author

Thanks for the feedback and the changes.

Some comments I forgot to add:

  • One of the things I had to change in the tests was toBe for toEqual, as the Proxy cannot intercept ===.
  • The arrow function is not really necessary.
  • Performance using proxies is lower, but this helps in places where we cannot modify the logic to add injectAsync.(That's my case)

@zheksoon
Copy link
Owner

@tripodsgames Hi! Could you please elaborate a bit more about your specific usecase to understand what's (maybe) missing in the implementation, or make it in a different way (for example, loop detection). Appreciate any help :) Thanks!

@tripodsgames
Copy link
Author

@zheksoon Unfortunately I'm out of time lately, but I'll collect the information and update the documentation

@zheksoon
Copy link
Owner

zheksoon commented Apr 25, 2024

@tripodsgames maybe just continue in the thread, I think we could come to better decisions together :)

What I don't like in the current implementation (the inject-lazy branch) is loop detection. It's a bit rough to detect cycles by limiting the number of transient resolutions, so need to think a bit more about it

@tripodsgames
Copy link
Author

tripodsgames commented Apr 26, 2024

@tripodsgames maybe just continue in the thread, I think we could come to better decisions together :)

What I don't like in the current implementation (the inject-lazy branch) is loop detection. It's a bit rough to detect cycles by limiting the number of transient resolutions, so need to think a bit more about it

I'm studying a way to eliminate the problem of cycles, I know it's possible

@tripodsgames tripodsgames changed the title injectLazy implementation. inject-lazy implementation. Apr 26, 2024
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.

2 participants