aurelia / aurelia/dependency-injection

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

Open
#197 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
159
Forks
64
PR merge metrics
No merged PRs in 30d

Description

**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
```typescript
// 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
```shell
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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with test/resolver.spec.ts and reproduce the two get-order cases from the issue. Run npm install followed by npm run test, then compare the behavior with the expected results: retrieving NewInstance.of(Logger) first must not make that object the container's default Logger instance, and both tests should pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
tooling
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.