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

Bug: @newInstance() becomes container default instance if it's the first instance #197

Open
krisdages opened this issue Feb 22, 2020 · 1 comment

Comments

@krisdages
Copy link

I'm submitting a bug report

  • Library Version:
    1.5.2

This is a regression from v1.4.2, where this worked correctly. I tried to implement the same test for that version, but had issues getting everything to build and run for that tag. The new TS repo structure and scripts are much appreciated. :)

Please tell us about your environment:

  • Operating System:
    Linux (Ubuntu 18.04)

  • Node Version:
    10.16

  • NPM Version:
    N/A
  • JSPM OR Webpack AND Version
    N/A
  • Browser:
    all

  • Language:
    all

Current behavior:
https://github.com/krisdages/aurelia-dependency-injection/tree/bugtest/new-instance-injects-default

// test/resolver.spec.ts
// PASSES, as expected
       it('get a new instance of a dependency, without regard for existing instances in the container', () => {
        const container = new Container();
        const logger = container.get(Logger);
        const newLogger = container.get(NewInstance.of(Logger));

        expect(logger).toEqual(jasmine.any(Logger));
        expect(newLogger).toEqual(jasmine.any(Logger));
        expect(newLogger).not.toBe(logger);
      });

//FAILS
      it('new instance of a dependency does not become the default instance in the container', () => {
        const container = new Container();
//only difference is the order of the gets.
        const newLogger = container.get(NewInstance.of(Logger));
        const logger = container.get(Logger);

        expect(logger).toEqual(jasmine.any(Logger));
        expect(newLogger).toEqual(jasmine.any(Logger));
        expect(newLogger).not.toBe(logger);
      });

Expected/desired behavior:
Both tests should pass.

clone or checkout https://github.com/krisdages/aurelia-dependency-injection

git clone https://github.com/krisdages/aurelia-dependency-injection
git checkout bugtest/new-instance-injects-default
npm install
npm run test
  • What is the expected behavior?

  • What is the motivation / use case for changing the behavior?
    This is a regression from version 1.4

@bigopon
Copy link
Member

bigopon commented Aug 10, 2024

Hmm, interesting, thanks and sorry for the wait @krisdages this' been missed.

I'm not sure if you are still interested, but I'll just comment for anyone who is

  //only difference is the order of the gets.
  const newLogger = container.get(NewInstance.of(Logger));
  const logger = container.get(Logger);

logger being the same instance with newLogger is the expected behavior in v1, where everything is automatically registered with the invoking container. It might have worked differently in 1.4.2, but it's been this way since, I'm not sure reverting it is a choice anymore. What we can do is to have a 2nd parameter on NewInstance.of() to indicate that it shouldn't result in a registration with the container, so your test will pass.

        const container = new Container();
        const newLogger = container.get(NewInstance.of(Logger, false)); // do not register
        const logger = container.get(Logger);

Though this probably will need to wait for someone who actually needs the behavior.

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